Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

Conversation

legalimpurity
Copy link
Contributor

@legalimpurity legalimpurity commented Mar 27, 2024

Which problem is this PR solving?

This is an implementation of JaegerRemoteSampler. This follows the specification mentioned here.

Fixes #4534

Short description of the changes

Add a new samper implementation in experimental packages.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Have added full coverage for entire code.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@legalimpurity legalimpurity requested a review from a team March 27, 2024 12:18
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.86%. Comparing base (2610122) to head (7f476b3).
Report is 12 commits behind head on main.

❗ Current head 7f476b3 differs from pull request most recent head b553fbe. Consider uploading reports for the commit b553fbe to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4589      +/-   ##
==========================================
+ Coverage   90.77%   92.86%   +2.08%     
==========================================
  Files          90      331     +241     
  Lines        1930     9571    +7641     
  Branches      417     2054    +1637     
==========================================
+ Hits         1752     8888    +7136     
- Misses        178      683     +505     
Files Coverage Δ
...s/sampler-jaeger-remote/src/JaegerRemoteSampler.ts 100.00% <100.00%> (ø)
...s/sampler-jaeger-remote/src/PerOperationSampler.ts 100.00% <100.00%> (ø)
...mental/packages/sampler-jaeger-remote/src/types.ts 100.00% <100.00%> (ø)
...y-sdk-trace-base/src/sampler/ParentBasedSampler.ts 78.78% <0.00%> (-5.09%) ⬇️
...trace-base/src/sampler/TraceIdRatioBasedSampler.ts 92.00% <0.00%> (-8.00%) ⬇️

... and 244 files with indirect coverage changes

@legalimpurity
Copy link
Contributor Author

Ah one npm run lint:fix is still needed - then this is good to go. 🙂

done.


2. **Probabilistic Sampling**: If the remote configuration specifies `StrategyType.PROBABILISTIC`, it creates a `TraceIdRatioBasedSampler`. This samples a percentage of traces based on the trace ID.

3. **Default Sampling**: If none of the above apply, it falls back to the initial sampler provided in the constructor.
Copy link
Member

@pichlermarc pichlermarc Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like lint is failing as it's missing the links used in the header. This is the footer we usually use that should fix it:

Suggested change
3. **Default Sampling**: If none of the above apply, it falls back to the initial sampler provided in the constructor.
3. **Default Sampling**: If none of the above apply, it falls back to the initial sampler provided in the constructor.
## Useful links
- For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
- For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
- For help or feedback on this project, join us in [GitHub Discussions][discussions-url]
## License
Apache 2.0 - See [LICENSE][license-url] for more information.
[discussions-url]: https://github.com/open-telemetry/opentelemetry-js/discussions
[license-url]: https://github.com/open-telemetry/opentelemetry-js/blob/main/LICENSE
[license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat
[npm-url]: https://www.npmjs.com/package/@opentelemetry/sampler-jaeger-remote
[npm-img]: https://badge.fury.io/js/%40opentelemetry%2Fsampler-jaeger-remote.svg

@pichlermarc pichlermarc added this pull request to the merge queue Aug 23, 2024
Merged via the queue into open-telemetry:main with commit c398601 Aug 23, 2024
19 checks passed
@legalimpurity legalimpurity deleted the feature/implement_jaeger_remote_sampler branch August 28, 2024 16:24
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Jaeger Remote Sampler as defined by the specification
5 participants