-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: add support for environment resource detector #29547
opentelemetry tracer: add support for environment resource detector #29547
Conversation
Add the extension interfaces and the environment resource detector. Signed-off-by: Joao Grassi <[email protected]>
When the OTel tracer starts, all the configured resource detector extensions are loaded and the resource object is constructed. The resource is then used when building the final OTLP request. Signed-off-by: Joao Grassi <[email protected]>
Hi @joaopgrassi, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
...ensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.h
Outdated
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD
Show resolved
Hide resolved
…detectors-env Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
Here's a first-pass review.
FWIW, it is better to create s series of smaller PRs to increase review speed.
source/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc
Outdated
Show resolved
Hide resolved
...nsions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc
Show resolved
Hide resolved
...nsions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc
Outdated
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/resource_detector.h
Outdated
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/resource_provider.cc
Outdated
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/resource_provider.cc
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/resource_provider.cc
Outdated
Show resolved
Hide resolved
source/extensions/tracers/opentelemetry/resource_detectors/resource_provider.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
@adisuissa I fixed the docs CI issues. Now the new proto is present in the API docs. Here are the changed/new pages:
Sorry for the size of the PR. I wasn't sure if I should break it only adding the detectors and later using. I will keep it in mind for the next ones :) |
/docs for ref @joaopgrassi you can get it to link the docs that it has built in ci - this gives a generic pr link which resolves to commit - you can link these for specific pages |
Docs for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/29547/docs/index.html The docs are (re-)rendered each time the CI |
Looks like a formatting issue? ./source/extensions/tracers/opentelemetry/resource_detectors/resource_detector.h:16: * @brief A strig key-value map that stores the resource attributes. |
Signed-off-by: Joao Grassi <[email protected]>
@RyanTheOptimist sorry, it was a typo. fixed! |
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
Looks like this needs a main merge |
…detectors-env Signed-off-by: Joao Grassi <[email protected]>
@RyanTheOptimist updated and solved the conflict. I gave write permission to maintainers on the branch/PR, in case it happens again. Should be good to go! |
/retest |
…detectors-env Signed-off-by: Joao Grassi <[email protected]>
Commit Message: Allow specifying resource detectors for the OpenTelemetry tracer via a new configuration
resource_detectors
. The resource detector reads from the env variableOTEL_RESOURCE_ATTRIBUTES
which is defined by the OTel specification. The detector returns a resource object populated with the detected attributes, which is sent as part of the OTLP request.Additional Description: This PR adds the "foundation" for building other resource detectors in Envoy. It is based on the OTel collector implementation. Users can configure multiple resource detectors, and they work together to "merge" all the detected attributes into a single resource object, which is then part of the OTLP message exported.
Risk Level: Low
Testing: Multiple unit tests, that cover all new code/scenarios. I also did manual testing, running Envoy locally with the OTel tracer + env resource detector enabled. Resource attributes detected from my environment is successfully exported as seen in the Jaeger screenshot.
Docs Changes: Not sure if I should add/where. Happy to do it.
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #28929]
Here is how the new config is used: