-
Notifications
You must be signed in to change notification settings - Fork 239
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
Make use_all config friendlier #1406
Conversation
|
There was a problem hiding this 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?
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 I do like the idea of log level to control it.
Reason to keep 1/3 separate is we should certainly continue to inform users if something were to go wrong. 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? |
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? |
c62ce87
to
5c973a5
Compare
@ahayworth switched to log level vs passing flag given its cleaner. |
5c973a5
to
975999f
Compare
975999f
to
a2fc833
Compare
@ahayworth bump :) |
There was a problem hiding this 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.
Upvote! Running into lots of warning messages on startup due to missing dependency and this change would be really useful :) |
7bf3189
to
ed03bc6
Compare
@ahayworth all good. Appreciate the review. |
There was a problem hiding this 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.
There was a problem hiding this 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.
ed03bc6
to
a52a678
Compare
a52a678
to
2e7c150
Compare
@robertlaurin @fbogsany bump, let's see if we can merge it before this PR's anniversary :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 LGTM
2e7c150
to
690048a
Compare
Helloooo a plea/nag... could this be looked again by the maintainers please? |
690048a
to
e69bcb3
Compare
e69bcb3
to
d9a262f
Compare
New year, new ping on this ;). Could this one be merged and released soon? Our customer highlighted again as a friction point. |
Set the log to debug given many dependencies will not be found and logs become noisy on boot.
d9a262f
to
b83e43c
Compare
is a new version of the opentelemetry-instrumentation-registry gem going to be released anytime soon? |
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