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

Questions about opentelemetry_absinthe #48

Open
calvin-kargo opened this issue Mar 1, 2022 · 13 comments
Open

Questions about opentelemetry_absinthe #48

calvin-kargo opened this issue Mar 1, 2022 · 13 comments

Comments

@calvin-kargo
Copy link

calvin-kargo commented Mar 1, 2022

Hi all, i am reaching out to you for asking about opentelemetry_absinthe​, but because i am not sure how to reach you all, i am posting this as an issue

I am currently working on my project on instrumenting services built using Phoenix, Ecto, Absinthe with Open Telemetry,
searching around the internet, currently opentelemetry_absinthe​ is the only open source tools for direct integration of absinthe​ and opentelemetry​.

However, i am a bit unsure and confused regarding opentelemetry_absinthe​, so i am reaching out to you as maintainer of opentelemetry_absinthe​, hoping you would help clarifying issues:

  • Almost all other instrumentation library is on https://github.com/open-telemetry/opentelemetry-erlang-contrib repo, what's the plan opentelemetry_absinthe​ ? is opentelemetry_absinthe​ is planned for long time support?
  • Regarding tracing resolver, is there plan to update opentelemetry_absinthe​ with resolver level tracing? it would be helpful to know which of the complex graphql field contributing a lot to latency issue.
  • absinthe 1.7.0​ and opentelemetry_telemetry​, a lot of other instrumentation library has been using opentelemetry_telemetry​ to help integration with telemetry based event (example: https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/main/instrumentation/opentelemetry_phoenix/lib/opentelemetry_phoenix.ex). opentelemetry_telemetry come with support of telemetry_span_context which enable multi-process tracing (so in theory we wouldn't need to do batch_keep_span​). absinthe​ itself has supported telemetry_span_context​ since 1.6.7​ and now 1.7.0​ . is there plan to migrate to opentelemetry_telemetry​ ?

In case you plan to add these features, and need help to implement it, I would be happy to help!

Thanks,

Calvin Sadewa

@calvin-kargo
Copy link
Author

I have forked this repo with change that is using opentelemetry_telemetry and trace additional absinthe event (resolve field, batch) on https://github.com/calvin-kargo/opentelemetry_absinthe

@tsloughter
Copy link

Hi, I was coming here to ask similar questions but an additional issue is the use of the name on hex, https://hex.pm/packages/opentelemetry_absinthe. The prefix opentelemetry_ is used by apps owned by the opentelemetry hex org and can be found in the -contrib repo. Another user was planning to submit their absinthe instrumentation library to contrib but we wouldn't be able to publish it to hex under the expected name.

It would be great if efforts could somehow be merged into a single library in the contrib repo and the opentelemetry_absinthe hex package.

@pdincau
Copy link
Contributor

pdincau commented Jun 6, 2022

Hi @tsloughter @calvin-kargo . Sorry if took a while to answer your messages but we forgot to integrate the notification hook and we lost the issues 🙇 . We were already planning to give away the ownership of the repo to the community, we are totally open to that approach. Can you guide us on that? Otherwise we can just rename the package on hex.

@tsloughter
Copy link

@pdincau great, thanks! How to move forward depends on how involved you want to be moving forward. We'd be happy to handle everything if you added opentelemetry as an owner on the hex package. You can also send a PR to https://github.com/open-telemetry/opentelemetry-erlang-contrib and be included in the CODEOWNERS, but not necessary if you just want to have us move it forward.

@pdincau
Copy link
Contributor

pdincau commented Jun 6, 2022

@tsloughter let me check how we can add opentelemetry as owner. No need to be in CODEOWNERS, so I guess no PR or repository ownership is required...correct?

@aj-foster
Copy link

Hi @pdincau,

In case you are unsure how to proceed, you can transfer ownership of the package on Hex.pm by running the following command:

mix hex.owner add opentelemetry_absinthe opentelemetry

@adammohammed
Copy link

Hi, thanks for creating this library, it does exactly what I want! Is there any update on transferring ownership? I'd be much more likely to recommend it if it lived under the control of the OpenTelemetry maintainers.

@cpiemontese
Copy link
Contributor

Hello!

Sorry for the hard necro but as you may have noticed we never managed to get this done 😞
We would finally like to transfer ownership and we tried running the command suggested by @aj-foster, but it seems like we need to actually call mix hex.owner transfer

mix hex.owner add opentelemetry_absinthe opentelemetry
Adding owner opentelemetry with ownership level full to opentelemetry_absinthe
Adding owner failed
Validation error(s)
  username: organization ownership can only be transferred, removing all existing owners

The only "problem" I see with this is that this repo is still under primait. Should we maybe move the code before transferring ownership?

@tsloughter
Copy link

Ah yea, orgs you have to transfer, you can have multiple maintainers but not org owners. But yes, that would be great to get it into the contrib repo.

@cpiemontese
Copy link
Contributor

Ok! So I guess the steps are

  • get it into the contrib repo
  • transfer ownership of the org

Does that sound good?

@tsloughter
Copy link

Sounds good.

@cpiemontese
Copy link
Contributor

Finally opened the PR: open-telemetry/opentelemetry-erlang-contrib#369

@tsloughter
Copy link

Great, thanks!

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

No branches or pull requests

6 participants