-
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 Dynatrace resource detector #30824
opentelemetry tracer: add Dynatrace resource detector #30824
Conversation
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Co-authored-by: Thomas Ebner <96168670+samohte@users.noreply.github.com> Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
96b7d5e
to
58ab437
Compare
…detector Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
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.
/lgtm api
…detector Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
Before the actual review, as this is a new extension, who will sponsor this extension? |
Hi @lizan ! I have been contributing to improve the OpenTelemetry tracing features in Envoy. Recently I got the foundation for the resource detector feature in the OTel tracer + an initial extension similar to this one, but that detects attributes from an standard environment variable. #29547 In those PRs, I was mainly in contact/sync with @htuch and @adisuissa. @htuch, @adisuissa would it work for one of you to be the sponsor of this one? It is similar to the environment resource detector, but this reads from the metadata files from Dynatrace. Thank you! |
I think @wbpcode could be a sponsor as he covers o11y broadly in the code base. Please consider me a sponsor of last resort, largely due to time constraints, not because of any stance on the extensions itself :) |
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.
First pass of review, mostly concerns around handling file I/O in data path.
cc @wbpcode if you're going to sponsor.
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Outdated
Show resolved
Hide resolved
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Show resolved
Hide resolved
...extensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_resource_detector.cc
Show resolved
Hide resolved
...extensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_resource_detector.cc
Outdated
Show resolved
Hide resolved
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Outdated
Show resolved
Hide resolved
Basically it's ok for me to sponsor this tracing-related extension. But I need some time to learn more context about this extension. And I am pretty busy this week. So, I can only put my time on this until next week. |
@wbpcode thank you! Feel free to dm me in Slack, if you want a "less async" discussion. |
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
Changes LGTM, defer to @wbpcode as extension sponsor. |
Will take a look next Monday. Sorry for the delay. |
please take a look @wbpcode 🌷 |
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.
The code looks good overall and some minor comments are added.
By the way, I am not very familiar to the OSS management. AFAIK, Dynatrace is a commercial production (?), is it OK to add this extension to Envoy? (No objection, just to confirm in case.) cc @htuch @alyssawilk
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Outdated
Show resolved
Hide resolved
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Outdated
Show resolved
Hide resolved
...extensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_resource_detector.cc
Outdated
Show resolved
Hide resolved
...extensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_resource_detector.cc
Outdated
Show resolved
Hide resolved
.../extensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_resource_detector.h
Show resolved
Hide resolved
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@wbpcode thanks for the review! I think I addressed everything. |
cc @joaopgrassi Thanks for you such quick response. Will take a final pass tomorrow after @htuch or @alyssawilk addressed my above question. If all goes well, this will be landed tomorrow or after tomorrow. :) |
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.
@wbpcode for your question about adding new extensions, we should go by https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md - if there's no maintainer to sponsor it should go in contrib.
Also I'm really confused why this didn't fail the presubmit checking new directories are added to CODEOWNERS with appropriate owners. https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md cc @phlax
Just as some more context for this comment from my side:
This extension only works in connection with the OpenTelemetry tracer - it enhances the OpenTelemetry spans with resource attributes, in this case specific for Envoy users that have Dynatrace deployed in their infrastructure. I added the foundation for this feature in #29547 which now allows any observability vendors, cloud provider etc to provide their resource detector extension. This is highly important to put the spans in context with resources (compute, host, process etc). |
I actually wanted to ask about the license. I know some OSSs may reject commercial products related extensions. But anyway, seems it's not a problem for Envoy. |
I think may be it is because these sub extension points (include samplers and resource detectors) have no independent extension directory and be treated as part of open telemetry tracer. (I personally think this is okay to allow some embedded simple extension point be part of exist extension.) |
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.
Thanks for your great contribution. 🌷 LGTM with on style comment. And please add one line change log for this patch.
cc @alyssawilk if you also think it's OK to place this extension at current position, I will merge this patch tomorrow (?).
...ensions/tracers/opentelemetry/resource_detectors/dynatrace/dynatrace_metadata_file_reader.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
…detector Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
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.
LGTM. Thanks. 🌷
Commit Message: Add Dynatrace resource detector. The resource detector reads from the Dynatrace enrichment files and returns a resource object populated with the detected attributes, which is sent as part of the OTLP request.
Additional Description:
When Dynatrace is deployed on the environment (either via or OneAgent, or via our K8s operator) certain files are made available to the process. These files are "virtual files", meaning they don't physically exists in the file system, but are provided by the OneAgent instrumentation on demand. That is why we couldn't use the existing file abstraction in Envoy, as those check for the existence of the files on disk.
Because Dynatrace can be deployed in many types of environments, some of these files are only present in such environments (e.g. k8s). Since the detector doesn't have means to know which environment it is running, it attempts to read all files, meaning some of them may not exist.
There could also be a case when a Dynatrace deployment is not successfull, causing no files to be available. In this case, the detector won't find any attribute and the resource is returned empty. We don't want to block Envoy from starting in this case, as Dynatrace never stops its customer's apps from running if a failure on our side happened. In such cases, customers can easily find out in Dynatrace which deployment failed and which host/environment is not properly monitored, allowing them to fix it.
This is a crucial step in adapting Dynatrace support for Envoy due to the deprecation of OpenTracing #28987. Only through such enrichment files, can Dynatrace customers get Envoy telemetry associated with their resources and put it into context (e.g. infrastructure)
Risk Level: Low
Testing: Multiple unit tests and a integration test that cover all new code/scenarios. I also did manual testing running Envoy in a environment with Dynatrace installed.
Docs Changes: Automatically generated (and verified) by new proto additions.
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #30823]
Here is how the new config is used: