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

opentelemetry tracer: implement TraceIDRatioBased and ParentBased samplers #37787

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

therealak12
Copy link
Contributor

Commit Message: implement TraceIDRatioBased and ParentBased samplers

Additional Description:
A previous issue existed for adding opentelemetry samplers to Envoy so I didn't create a new one. The PR at hand implements two of these samplers.
The implementation and testing strategies are inspired by the OTEL's Go and CPP SDKs.

Risk Level: Low

Testing: The PR has unit and integration tests which I've run locally.

Docs Changes:
Release Notes:
Platform Specific Features:

@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Dec 21, 2024
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #37787 was opened by therealak12.

see: more, trace.

@therealak12 therealak12 force-pushed the samplers branch 6 times, most recently from e186869 to b6bfb74 Compare December 21, 2024 13:13
@therealak12
Copy link
Contributor Author

therealak12 commented Dec 21, 2024

The ci/do_ci.sh coverage job is failing. The only unusual log entry I can find is a warning message. I'd appreciate any assistance in identifying and resolving the issue.

Here's the warning message I mentioned.

Screenshot from 2024-12-21 19-13-02

@phlax
Copy link
Member

phlax commented Dec 21, 2024

the warning is not relevant

this is the important line

Code coverage for source/extensions/tracers/opentelemetry/samplers/trace_id_ratio_based is lower than limit of 96.6 (84.8)

/coverage

Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/37787/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #37787 (comment) was created by @phlax.

see: more, trace.

@therealak12 therealak12 marked this pull request as draft December 21, 2024 17:33
@therealak12
Copy link
Contributor Author

therealak12 commented Dec 21, 2024

Thanks for your guidance, dear @phlax. Tests are passing now.

@therealak12 therealak12 marked this pull request as ready for review December 21, 2024 18:18
@wbpcode
Copy link
Member

wbpcode commented Dec 22, 2024

Thanks for the contribution. Please read the https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md to get more infomation about adding new extensions to see if the new extensions meet the requirement or not.

Thanks again!

@therealak12
Copy link
Contributor Author

Please read the https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md to get more information about adding new extensions to see if the new extensions meet the requirement or not.

Thanks for the links, @wbpcode! I’ve reviewed them and made the necessary adjustments to my PR. However, just to clarify, my PR doesn't add a new extension; rather, it implements two missing samplers for the OpenTelemetry tracer extension.

Could you please let me know if that matches your expectations?

@therealak12 therealak12 changed the title Implement TraceIDRatioBased and ParentBased samplers opentelemetry tracer: implement TraceIDRatioBased and ParentBased samplers Dec 22, 2024
@therealak12
Copy link
Contributor Author

If you get a chance, I'd appreciate any feedback or comments when you have time. No rush, I understand you're busy.

@wbpcode
Copy link
Member

wbpcode commented Dec 25, 2024

Thanks for the links, @wbpcode! I’ve reviewed them and made the necessary adjustments to my PR. However, just to clarify, my PR doesn't add a new extension; rather, it implements two missing samplers for the OpenTelemetry tracer extension.
Could you please let me know if that matches your expectations?

Sorry for the delay. I skipped messages from community these two days.

It's no doubt this add some new extensions. Although these extensions are not top level extensions but sub extensions for opentelmetry. But it doesn't change to much the characteristics.

So, at least, we should ensure the current owners of opentelmetry @AlexanderEllis @yanavlasov are willing to maintain these new two extensions.

@wbpcode
Copy link
Member

wbpcode commented Dec 25, 2024

/wait-any

@therealak12
Copy link
Contributor Author

I would appreciate it if you took a look at this when you have a moment.
@AlexanderEllis @yanavlasov

@wbpcode wbpcode added the waiting label Jan 6, 2025
@therealak12
Copy link
Contributor Author

Have you had a chance to review this PR? It's crucial for our tracing implementation.
@AlexanderEllis @yanavlasov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deps Approval required for changes to Envoy's external dependencies waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants