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

Attach to subscription publish events #145

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aej
Copy link

@aej aej commented Apr 10, 2024

I noticed that this library doesn't currently attach to the [:absinthe, :subscription, :publish] events. I'm not sure if this is the best way of doing it but wanted to put up a PR to at least start a discussion.

As you can see in the code ive added a custom attribute called "graphql.event.type" which is either "operation" or "publish".

"publish" events are essentially any time absinthe telemetry executes [:absinthe, :subscription, :publish].

For now this solves the problem for me but I'd be very happy to hear alternative ideas or suggestions on this.

Thanks!

@aej aej requested a review from a team as a code owner April 10, 2024 17:38
Copy link
Member

@MaeIsBad MaeIsBad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for your contribution! This looks great, I'd be happy to merge this, there are just a few things that need to be done:

  • update the changelog(feel free to do a minor version bump while you're at it, so I can release once merged)
  • since we're now also covering subscriptions I'd like to have some tests using them, so we can make sure other attributes function as expected with them

If you don't feel up for making these changes let me know I can take over on this

@@ -99,6 +115,7 @@ defmodule OpentelemetryAbsinthe.Instrumentation do
{:"graphql.request.variables", Jason.encode!(variables)}
)
|> put_if(config.trace_request_query, {@graphql_document, document})
|> put_if(true, {:"graphql.event.type", config.type})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be

Suggested change
|> put_if(true, {:"graphql.event.type", config.type})
|> List.insert_at(0, {:"graphql.event.type", config.type})

@aej
Copy link
Author

aej commented Apr 11, 2024

Hey, thanks for your contribution! This looks great, I'd be happy to merge this, there are just a few things that need to be done:

  • update the changelog(feel free to do a minor version bump while you're at it, so I can release once merged)
  • since we're now also covering subscriptions I'd like to have some tests using them, so we can make sure other attributes function as expected with them

If you don't feel up for making these changes let me know I can take over on this

Thanks @MaeIsBad , that’s all great feedback. If you don’t mind could I ask you to take this over? That would be very much appreciated

@cpiemontese
Copy link
Contributor

Hey @aej! We released a 2.3.0-rc.0 version, do you think you'd be able to test it and let us know if it works...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants