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

Add pre and post instrument entrypoints #1983

Merged
merged 6 commits into from
Sep 9, 2021

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jul 23, 2021

Fixes #1982

@ocelotl ocelotl self-assigned this Jul 23, 2021
@ocelotl ocelotl requested a review from a team as a code owner July 23, 2021 13:23
@ocelotl ocelotl requested review from aabmass and lzchen and removed request for a team July 23, 2021 13:23
@ocelotl ocelotl force-pushed the issue_1982 branch 2 times, most recently from be75fcd to 70dabd7 Compare July 23, 2021 13:34
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

I think @owais made a good point in the SIG. Should we make this PR additive of the new entry points only and put a comment that opentelemetry_distro entrypoint is deprecated?

Could you also add some documentation on the opentelemetry-{pre,post}-instrument entry points since this is general purpose and anyone can use it. Maybe some of the figures from the slides you presented too?

@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 8, 2021

I think @owais made a good point in the SIG. Should we make this PR additive of the new entry points only and put a comment that opentelemetry_distro entrypoint is deprecated?

I don't intend to deprecate opentelemetry_distro (I have renamed opentelemetry_distro to opentelemetry_configurator in #1966 because I found the latter name more representative than the first one, but it can be kept as opentelemetry_distro if the community prefers).

@owais
Copy link
Contributor

owais commented Aug 19, 2021

@ocelotl I'm totally onboard with this PR. I was only suggesting we make this change gradually by keeping the distro hook as is for now and only adding pre/post hooks as new hooks. Once the configurator package you are working on that will provide distro hooks is ready and published, we can remove the distro hook from this package. It's still the same design as you presented but will be done in may be one additional PR.

@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 25, 2021

Refactored this PR to only include the entry points

@ocelotl ocelotl requested review from aabmass and owais August 25, 2021 20:58
@owais
Copy link
Contributor

owais commented Aug 26, 2021

We need changelog for this.

@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 26, 2021

We need changelog for this.

added it 👍

@owais owais enabled auto-merge (squash) September 9, 2021 16:42
@owais owais disabled auto-merge September 9, 2021 17:16
@owais owais enabled auto-merge (squash) September 9, 2021 17:16
@owais owais merged commit 08392c8 into open-telemetry:main Sep 9, 2021
@ocelotl ocelotl deleted the issue_1982 branch September 24, 2021 16:15
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.

Distros are coupled with instrumentations
4 participants