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

Have Span::End (optionally) return the Duration (+start/end timestamp) of the span. #3951

Closed
meastp opened this issue Mar 21, 2024 · 6 comments
Labels
spec:trace Related to the specification/trace directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted triage:deciding:tc-inbox Needs attention from the TC in order to move forward

Comments

@meastp
Copy link

meastp commented Mar 21, 2024

What are you trying to achieve?
I need to extract the span duration to be able to generate a metric http_server_request_duration that, according to the semantic conventions, should have the same duration as the span/request.

My problem is that the SDK I use doesn't provide a method to read the span duration after it has ended. The arcihtecture chosen prevents being able to read span metadata, due to performance optimizations.

The SDK authors (@lalitb) preferred this go through the specification repo.

Change in the specification
As the duration, start and end time are the only(?) internally generated metadata, I was hoping that Span::End could be enhanced to return this metadata. The return type for Span::End isn't mentioned in the spec today, so it would be a backward compatible change.

It might be worded so SDKs that do provide other means to read the duration/start time/end time aren't required to do this modification.

Even though I only require the duration, either the start time + duration, end time + duration or start time + end time would be required to have all data available to the client.

What do you think?

@meastp meastp added the spec:trace Related to the specification/trace directory label Mar 21, 2024
@trask trask added triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted triage:deciding:tc-inbox Needs attention from the TC in order to move forward labels Mar 26, 2024
@dyladan
Copy link
Member

dyladan commented Mar 26, 2024

I support this. In the gc triage meeting @trask mentioned a use that the java instrumentation has that they want to create a span, then also create a metric that records the duration of the span. In that case, they need to get the duration of the span in order to make the metric match it exactly. Java instrumentation currently works around this by providing start and end times to span start and end. I think this is a great way to avoid instrumentation authors having to manage their own clocks (can tell you in JS this is a very fraught area).

@trask
Copy link
Member

trask commented Mar 26, 2024

@dyladan oops, what I said on our triage call wasn't correct, and Java instrumentation does (still) suffer the problem described in this issue where the metric duration doesn't (always exactly) match the span duration: open-telemetry/opentelemetry-java-instrumentation#5905

@meastp
Copy link
Author

meastp commented Apr 8, 2024

@trask @dyladan ok, so there is some interest beyond otel-cpp. great :)

what is the next step here? draft a spec change as a PR or is whether to proceed with this spec change something you decide by other means?

@jack-berg
Copy link
Member

Discussed in 4/10/24 TC meeting. We agreed that having synchronization between span and metric duration is important, but don't think this proposal of making the duration accessible to span API callers is the appropriate way to solve:

  • It's a non-starter for the noop implementation, which would either have to maintain state to fulfill this contract or return incorrect values.
  • Its already possible for a caller to explicitly set the span start and end time to get the desired result.

Instead, we might consider taking a step further than defining some SDK capability for computing metrics from spans, potentially via a span processor. I'm going to close this issue, but feel free to open if you think we got it wrong.

@meastp
Copy link
Author

meastp commented Apr 11, 2024

@jack-berg ok, no worries - a long term solution of computing the metric in the SDK is probably better (and less work when instrumenting manually :) ).

@dyladan
Copy link
Member

dyladan commented Apr 16, 2024

Discussed in 4/10/24 TC meeting. We agreed that having synchronization between span and metric duration is important, but don't think this proposal of making the duration accessible to span API callers is the appropriate way to solve:

  • It's a non-starter for the noop implementation, which would either have to maintain state to fulfill this contract or return incorrect values.
  • Its already possible for a caller to explicitly set the span start and end time to get the desired result.

Instead, we might consider taking a step further than defining some SDK capability for computing metrics from spans, potentially via a span processor. I'm going to close this issue, but feel free to open if you think we got it wrong.

@jack-berg I think for the usecase mentioned by @trask it can still work if no-op would just return a nonsense value like -1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted triage:deciding:tc-inbox Needs attention from the TC in order to move forward
Projects
None yet
Development

No branches or pull requests

5 participants