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

Allow customization of parent-override behaviour for inferred-spans #1533

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

JonasKunz
Copy link
Contributor

Description:

The inferred spans extension sometimes has to insert inferredspans inbetween the parent-child relationship of normal spans for correctness.

This is however not directly possible, that is why we represent this as a span-link on the inferred spans. This PR adds a configuration hook to fully customize this behaviour with a custom handler.

The main reason for adding this is because in our elastic vendor distro of the opentelemetry agent we are still shipping the pre-contribution version of inferred spans. We currently still have to do this, because there these special span links are marked with an elastic.is_child instead of just is_child attribute and older versions of our backend rely on this elastic-specific attribute.

With this change we can switch to this contributed version of inferred spans and add the elastic.is_child attribute on the span link via the customization hook.

@trask trask added this to the v1.41.0 milestone Nov 18, 2024
@@ -41,6 +41,8 @@ So if you are using an autoconfigured OpenTelemetry SDK, you'll only need to add
| otel.inferred.spans.interval <br/> OTEL_INFERRED_SPANS_INTERVAL | `5s` | The interval at which profiling sessions should be started. |
| otel.inferred.spans.duration <br/> OTEL_INFERRED_SPANS_DURATION | `5s` | The duration of a profiling session. For sampled transactions which fall within a profiling session (they start after and end before the session), so-called inferred spans will be created. They appear in the trace waterfall view like regular spans. <br/> NOTE: It is not recommended to set much higher durations as it may fill the activation events file and async-profiler's frame buffer. Warnings will be logged if the activation events file is full. If you want to have more profiling coverage, try decreasing `profiling_inferred_spans_interval` |
| otel.inferred.spans.lib.directory <br/> OTEL_INFERRED_SPANS_LIB_DIRECTORY | Defaults to the value of `java.io.tmpdir` | Profiling requires that the [async-profiler](https://github.com/async-profiler/async-profiler) shared library is exported to a temporary location and loaded by the JVM. The partition backing this location must be executable, however in some server-hardened environments, `noexec` may be set on the standard `/tmp` partition, leading to `java.lang.UnsatisfiedLinkError` errors. Set this property to an alternative directory (e.g. `/var/tmp`) to resolve this. |
| otel.inferred.spans.duration <br/> OTEL_INFERRED_SPANS_DURATION | `5s` | The duration of a profiling session. For sampled transactions which fall within a profiling session (they start after and end before the session), so-called inferred spans will be created. They appear in the trace waterfall view like regular spans. <br/> NOTE: It is not recommended to set much higher durations as it may fill the activation events file and async-profiler's frame buffer. Warnings will be logged if the activation events file is full. If you want to have more profiling coverage, try decreasing `profiling_inferred_spans_interval` |
| otel.inferred.spans.parent.override.handler <br/> OTEL_INFERRED_SPANS_PARENT_OVERRIDE_HANDLER | Defaults to a handler adding span-links to the inferred span | Inferred spans sometimes need to be inserted as the new parent of a normal span, which is not directly possible because that span has already been sent. For this reason, this relationship needs to be represented differently, which normally is done by adding a span-link to the inferred span. This configuration can be used to override that behaviour by providing the fully qualified name of a class implementing `BiConsumer<SpanBuilder, SpanContext>`: The biconsumer will be invoked with the inferred span as first argument and the span for which the inferred one was detected as new parent as second argument |
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you could use ComponentProvider, which is how file configuration is supporting configuring things like this (instead of fully-qualified class name)

cc @jack-berg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually this definitely would make sense I think. However, is the new structured config API already in a state so that we can fully replace the usage of the AutoConfigurationCustomizerProvider here? Because I don't think that we can simply mix in the ComponentProvider to my understanding

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

I sent a PR to your branch just to demonstrate what I was thinking: JonasKunz#1

but it does require depending on a bunch of internal / experimental classes, so really not sure if it's worth it yet

Copy link
Member

Choose a reason for hiding this comment

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

ComponentProvider is for providing implementations of SDK plugin extension points referenced in the declarative confg files. I.e. samplers, processors, exporters, etc.

@trask it looks like you're using it to provide an implementation of ParentOverrideHandler (not an SDK plugin extension point), and using outside of declarative config.

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah, I was thinking maybe we could use component provider for custom declarative config stuff, but I guess declarative config has no way to know about what nodes represent components and what provider types to use

@trask trask merged commit 6ca397a into open-telemetry:main Nov 20, 2024
14 checks passed
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.

5 participants