Skip to content

Conversation

@fcevado
Copy link
Contributor

@fcevado fcevado commented Jun 25, 2022

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@fcevado fcevado changed the title opentelemetry_ecto - add possibility to setup additiona attributes. opentelemetry_ecto - add possibility to setup additional attributes. Jun 25, 2022
@fcevado fcevado requested a review from bryannaegele June 25, 2022 22:32
@andrewhr
Copy link
Contributor

Isn't this task better suited for #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?

@tsloughter
Copy link
Member

@andrewhr that would assume they want the attributes to also be in baggage.

@fcevado
Copy link
Contributor Author

fcevado commented Jun 28, 2022

@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.

@fcevado
Copy link
Contributor Author

fcevado commented Aug 25, 2022

@tsloughter @bryannaegele any update on this?

@tsloughter
Copy link
Member

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.

@bryannaegele
Copy link
Contributor

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?

@bryannaegele
Copy link
Contributor

Also, @fcevado you still need to sign the CLA.

@tsloughter
Copy link
Member

Looks ok to me.

@fcevado
Copy link
Contributor Author

fcevado commented Sep 2, 2022

@bryannaegele it's signed 👍

@github-actions github-actions bot removed the scope-ci label Sep 2, 2022
@bryannaegele bryannaegele merged commit a9e6fa0 into open-telemetry:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants