Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: 🦠 Metrics prototype for Rack instrumentation #1129
base: main
Are you sure you want to change the base?
feat: 🦠 Metrics prototype for Rack instrumentation #1129
Changes from all commits
2e6b9ab
8338fc4
b031ceb
ed4a354
a38aa87
951e0af
92d9ef6
8b4554f
ff2fbdb
3eef72a
258b587
ba9d06d
7f8ce16
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I have a bunch of ideas and questions floating around in my head that could use your input being a metrics SDK expert.
It may result in a separate dispatch call, but have you considered creating a separate handler for metric generation? That would remove the need for having conditionals in the tracer middleware.
What if our user has an o11y 1.0 mindset and only wanted to generate metrics and not traces? Would that not mean the span would be non_recording and the metric would always generate invalid histogram entries?
Is there anything in the spec that states this should be generated from span timestamps?
Is there an alternative implementation that generates metrics in the span processor pipeline?
E.g. there is a a processor that implements on_finish generated metrics for server and client spans regardless of the instrumentation?
What about one that decorates the BatchSpanProcessor export loop and generates metrics in the bsp thread to minimize the metrics from being generated in the critical path of the users request?
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.
I don't think there is spec about where we should generate metrics. afaik, metrics spec only talks about what the metrics should looks like and how to export them.
To generate the metrics from span processor, then the processor need have meter and create the instrument in processor in on_start or on_finish, I am thinkhing something like this?
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.
Perhaps.
What are other language sigs doing?
Is what I'm suggesting seemingly out of touch with what is the consistent pattern across language implementations?
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.
Love these questions! I have some research to do to follow-up, but here are my current thoughts.
The instrumentation is based on Python's approach with the aiohttp-server. The asgi instrumentation has an example of what we could do when
OTEL_SEMCONV_STABILITY_OPT_IN
comes around.I think we could take that approach! The examples I've seen so far tend to record the metric as part of the span creation, but we don't have to and that might be safer until metrics are stable.
This is a great thing to consider. I think that'd be the case with this current implementation, and I'd like to make some tweaks for this scenario.
I don't see an option to create only metrics and not traces in the Python instrumentation, but perhaps this exists in other languages. I'll do a bit more digging. My read on the spec is that it should leverage the span if it's available, but if the span is not available, it should still record the metric. spec
The spec says: "When this metric is reported alongside an HTTP server span, the metric value SHOULD be the same as the HTTP server span duration."
AFAIK, we don't save duration on a span, just a start and end timestamp, so I figured this would be the way to create a duration that's the same as the HTTP server span duration. I might be interpreting the spec incorrectly. We could get the start and end timestamps in an independent fashion. That may be closer to what Python does.
I haven't seen this, and this isn't something I considered. I'll poke around other languages and see if I can find an example. I haven't looked too much at the BSP metrics, I'll take a look to get a better sense of how they work.
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.
@arielvalentin - Thanks for your patience!
I spent some time looking at metrics instrumentation in other languages. The most common design I found was to create metrics within the same instrumentation file as spans, but to calculate the duration and collect the attributes in separate ways. It seems like metrics are stable in those languages, so this might not have always been the case.
When it comes to creating a separate handler for metrics, I don't feel strongly one way or the other. Would you prefer to keep them in separate handlers?
Here are some of the files I looked at. The links should be to the code that records the metric. In most cases, the same file also creates the histogram.
For the span processor question, I didn't come across use of a span processor to collect metrics. Since span processors are part of the SDK and not the API, and instrumentation should only rely on the API this doesn't seem like quite the right fit. In addition, since spans and metrics should be independent of each other, I don't think we want a span processor to control the creation of another signal. What do you think?
It looks like there are some features coming with the meter configurator that allow users to control what data is collected by instrumentation scope. This could help someone control what telemetry they want from a given instrumentation scope, and support that Olly 1.0 mindset of sending only metrics, but not traces. I'll look into prototyping the experimental spec and use that to control whether metrics and traces are sent. We could probably do this with configs in the instrumentation too.
So as a follow-up, I'd like to:
Let me know if there's anything I missed! Thanks again for your feedback!
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.
@arielvalentin @xuan-cao-swi - I have a prototype ready that creates metrics in a separate event handler: #1213
I decided to put it in a different PR to start to make it easier to compare the two approaches.
Next on my list is to prototype the metrics configuration work, but that PR will likely live in the core repo.