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 OpenTelemetry client instrumentation #108

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

genebean
Copy link
Contributor

@genebean genebean commented Oct 10, 2020

Status

In Progress

Description

Add OpenTelemetry client instrumentation

Related Issues

Todos

  • Tests
  • Documentation

Reviewers

@puppetlabs/dio
@highb
@briancain

@genebean genebean changed the title ## Status Add OpenTelemetry client instrumentation Oct 10, 2020
Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

At the very least, this should be opt-in 🙈 And it shouldn't exit on any failure raised by the telemetry class.

I'm not a huge fan of what this change means personally, and I feel like it would be worth letting people know how to disable it or opt in if it's off by default.

What's the reasoning behind adding tracking to this tool?

@genebean
Copy link
Contributor Author

@briancain It will absolutely be opt it, full stop. The default functionality will be to emit nothing and to stay out of the way.

The reasoning for this is that OpenTelemetry tracing has been added to VMPooler, NSPooler, and ABS and there is benefit in extending the trace information down to the initiating client.

This PR is still a draft because I still have a good bit of work to do to make sure it works as intended and that it really does stay out of the way. This is the first client-side instrumentation I have worked on and the first for an app that can also be used as a library a la what is done in puppetlabs/beaker-abs#18.

@briancain
Copy link
Contributor

@genebean Great! Thanks for the extra info. Also apologies if I came on a little strong this morning ❤️ That makes sense, it sounds like there's some confusion around what tool is making what requests.

@highb gave me a bit more context too.

@genebean
Copy link
Contributor Author

No problem at all @briancain!

@highb
Copy link
Contributor

highb commented Oct 12, 2020

Just chiming in to say that getting this trace data should make debugging failed VM checkouts across Beaker to ABS to VMPooler/AWS/VSphere so much easier. I'm happy to see this work moving forward and looking forward to seeing how it evolves (even if I can't really see it in action anymore 😢🎻 )

That being said, I'd agree with @briancain that it should definitely be opt-in/no impact on users who don't want it, and that's how it looks to be configured currently. I'm sure @genebean is being very conscientious about user privacy here. ❤️

@highb
Copy link
Contributor

highb commented Dec 12, 2020

@genebean Happy to review this again whenever you're ready.

@highb
Copy link
Contributor

highb commented Jul 13, 2021

@genebean Any progress here?

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.

3 participants