-
Notifications
You must be signed in to change notification settings - Fork 208
add OpenTelemetry instrumentation to titiler.core #1171
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
Conversation
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.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'TiTiler performance Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.
| Benchmark suite | Current: df331c6 | Previous: 995e833 | Ratio |
|---|---|---|---|
WGS1984Quad longest_transaction |
0.07 s |
0.05 s |
1.40 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Thanks @hrodmn this looks super promising 🙏 I've added couple comments (mostly on naming). My main concern is about the number of lines the tracing will add to the factory, mostly due to the If we move forward, we would need to add trace to all other factory endpoints 😬 |
Yes, I agree about not wanting to add so many lines just for tracing. I will work on ways to make attribute setting more succinct. User input parameters should be easy to absorb automatically but anything else would need to be added manually. Maybe we can leave
Thanks to the |
4a86484 to
8b8a9b8
Compare
|
@vincentsarago thanks for your feedback on these changes! I have implemented your naming suggestions and trimmed out the verbose attribute tracing lines. I did some research on the expected performance hit for including these traces and I don't think it will be significant. Using the |
Historically OTEL has had some bad performance issues but most of that has been worked out over the last few years 👍 |
e1d2a27 to
5786e36
Compare
a07dfeb to
15b75f4
Compare
8ca0685 to
ed338c0
Compare
geospatial-jeff
left a comment
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.
🚀

From #1167
This adds some convenience functions for adding opentelemetry traces to titiler applications in titiler.core.telemetry. These features will only be activated if the opentelemetry dependencies are installed.
When enabled in a titiler application, the traces can be browsed in a UI like Jaeger:

Factory class instrumentation: the
BaseTilerFactoryhas a new methodadd_telemetry()which will add a trace to each registered endpoint in the factory class.Endpoint instrumentation: endpoint methods in all of the tiler factory classes in
titiler.core,titiler.mosaic, andtitiler.xarrayhave been decorated with someoperation_tracercontext managers to track the performance of specific operations within an endpoint function.To keep things clean I opted to skip adding additional attributes to the traces, but we might want to revisit that in the future!
TODO