opentelemetry-erlang-contrib
opentelemetry-erlang-contrib copied to clipboard
open-telemetry
opentelemetry_ecto - add possibility to setup additional attributes.
current implementation of opentelemetry_ecto doesn't allow to setup additional attributes to the span. this PR add this possibility by allowing an :attributes option on setup.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: fcevado / name: cevado (a211c62cb4fc537881de74f2048bd89bb4e31f92, 13e7acb1213c569e82ac241d6961137b35f97d2a)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Isn't this task better suited for https://github.com/open-telemetry/opentelemetry-erlang-contrib/pull/85?
The concern is this become a pattern for all instrumentation libraries. Not sure how it's the vision for that, specially after the Instrumentation Scope changes. Thoughts @tsloughter?
@andrewhr that would assume they want the attributes to also be in baggage.
@andrewhr as I understand baggage would be generic for the whole app not particular to a library context. for example, if I want to identify write and read replicas, I can specify different telemetry_prefix for the repos and setup the instrumentation with specific attributes to them. the same for applications that connect to multiple databases and i want something more descriptive than the db.url.
I think this is needed but I don't want to be the decided on a change like this to the Ecto integration.
But I'll give an argument for it in principle: in other cases, like an HTTP server span, you can add attributes to the Span the instrumentation creates because the user is writing the code that is being wrapped in a span (so simply call the set_attribute macro on the current context). But in the case of an instrumentation like Ecto the control is never given to the user because it simply runs the query.
Sorry. I was on vacation when you were still working through those conversations.
I think this is fine at this point? The functionality is required by the spec now. Did you have any specific objection to the implementation @tsloughter?
