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.

Sign up for GitHub

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

Jump to bottom

Make use_all config friendlier #1406

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Make use_all config friendlier #1406

merged 1 commit into from
Jan 19, 2024

Conversation

cwjenkins
Copy link
Contributor

Add new flag, suppress_not_found, to silence instrumentation that isn't found.
Separate out 'failed to install' from 'not found' in logs during install_all method

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 27, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: cwjenkins / name: Colton Jenkins (b83e43c)

ahayworth
ahayworth reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

Things I like: a more detailed error message! 🎉
Things I'm not sure about: should this be an option we ask users to pass in? Or is there a better way to do it?

I think I'm wondering specifically if we shouldn't do:

  • logger.info "installed successfully!"
  • logger.debug "failed to install... #{reason}" kind of thing

Basically I don't know that anyone ever has the full set of dependencies for instrumentation-all available. I presume that everyone would want to use this option (so maybe we should default this to true instead of false).

Thoughts?

@cwjenkins
Copy link
Contributor Author

Thanks for the feedback @ahayworth.

Personal preference, when starting out using something new I like it noisy. This is why I went with 'false'.

Example, I'm new to an app, I start with use_all given I'm unsure what's avail, this informs me what instrumentation was added and after such I can do something to quiet it down. I just learned one app has more than enough http clients than it should.

I do like the idea of log level to control it.
Seems like we should have 3 here.

  1. debug for 'not installed due to missing dependency'
  2. info for 'instrumentation 'x' installed with 'y' options'
  3. warn or maybe error for 'failed to install due to reason #{y}'

Reason to keep 1/3 separate is we should certainly continue to inform users if something were to go wrong.
Example, I recently wrapped up my config in a railtie and set it to configure 'after' the app initialized. This led to action pack instrumentation to fail given the middleware array was frozen at this point. If I had set my log level to info I wouldn't have caught it until later.

If log level defaults to info and informing users upon first use of what wasn't installed due to a missing dependency can't be done due to it being debug level, maybe we can add another info log that mentions 'if you want to see what wasn't included set log level to debug'.

Thoughts?

@ahayworth
Copy link
Contributor

The only thought that immediately comes to mind is "warn" is better than "error" I think, but ... I'm not sure otherwise. @open-telemetry/ruby-maintainers @open-telemetry/ruby-approvers any thoughts?

@cwjenkins
Copy link
Contributor Author

@ahayworth switched to log level vs passing flag given its cleaner.
If good, will squash.

@cwjenkins
Copy link
Contributor Author

@ahayworth bump :)

ahayworth
ahayworth approved these changes Mar 18, 2023
View reviewed changes
Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! Apologies for the delay, my work responsibilities have shifted so I haven't been super active around otel-ruby recently. I'm but a lowly approver on this repo, so we will need someone from @open-telemetry/ruby-maintainers to take a look.

@cheempz
Copy link

cheempz commented Apr 6, 2023

Upvote! Running into lots of warning messages on startup due to missing dependency and this change would be really useful :)

@cwjenkins cwjenkins force-pushed the make_use_all_friendlier branch 2 times, most recently from 7bf3189 to ed03bc6 Compare April 9, 2023 19:31
@cwjenkins
Copy link
Contributor Author

@ahayworth all good. Appreciate the review.
@dazuma @robbkidd @ericmustin any thoughts on this PR?

fbogsany
fbogsany approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

This LGTM. I'd like to get a 👍 from @robertlaurin before I merge - he's the expert on the Registry & instrumentation config code.

robbkidd
robbkidd approved these changes Jun 22, 2023
View reviewed changes
Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

👍🏻 Big fan of more nuanced console messaging.

@cwjenkins
Copy link
Contributor Author

@robertlaurin @fbogsany bump, let's see if we can merge it before this PR's anniversary :)

simi
simi approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@simi simi left a comment

Choose a reason for hiding this comment

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

🙏 LGTM

@cheempz
Copy link

cheempz commented Sep 1, 2023

Helloooo a plea/nag... could this be looked again by the maintainers please?

@cheempz
Copy link

cheempz commented Jan 16, 2024

New year, new ping on this ;). Could this one be merged and released soon? Our customer highlighted again as a friction point.

@cwjenkins
Add a log that states when a dependency isn't found versus installed.
b83e43c 
Set the log to debug given many dependencies will not be found and
logs become noisy on boot.
@mwear mwear merged commit e7d4545 into open-telemetry:main Jan 19, 2024
55 checks passed
@cwjenkins cwjenkins deleted the make_use_all_friendlier branch January 19, 2024 17:19
@ykitamura-mdsol
Copy link

is a new version of the opentelemetry-instrumentation-registry gem going to be released anytime soon?

@arielvalentin arielvalentin mentioned this pull request Mar 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simi simi simi approved these changes

@robbkidd robbkidd robbkidd approved these changes

@fbogsany fbogsany fbogsany approved these changes

@ahayworth ahayworth ahayworth approved these changes

@mwear mwear mwear approved these changes

@robertlaurin robertlaurin robertlaurin approved these changes

@kaylareopelle kaylareopelle kaylareopelle approved these changes

@dazuma dazuma Awaiting requested review from dazuma dazuma is a code owner

@ericmustin ericmustin Awaiting requested review from ericmustin ericmustin is a code owner

@arielvalentin arielvalentin Awaiting requested review from arielvalentin arielvalentin is a code owner

@plantfansam plantfansam Awaiting requested review from plantfansam plantfansam is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@cwjenkins @ahayworth @cheempz @ykitamura-mdsol @simi @robbkidd @fbogsany @mwear @robertlaurin @kaylareopelle