-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[extension/sumologicextension] removing collector name from credential path for sumologic #42453
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
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
echlebek
left a comment
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 PR needs to more carefully describe what the change is, and its impacts. Please post the design document in the PR description and include the background motivation for this change.
|
Please file an issue that this PR can resolve. Please include a changelog entry. |
echlebek
left a comment
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 are looking generally good to me code-wise. However I am not sure about logging the hash key. I think it would be better to avoid doing that.
|
@pankaj101A I think we're missing a few scenarios here. The collector name is currently tied to the registration process (probably should have been a UID instead). Changing the collector name without re-registering it may cause some of the metadata like Also, The collector name can currently be set in the sumo extension and is current part of the registration request payload. What happens in a re-registration cycle if we allowed a user to change the collector name outside registration? Would it override the manual change? Here's the registration payload, which sets collector fields and the timezone along with the collector name: I don't think we should allow a customer to change the collector name without re-registration, unless we understand the full implications of doing so or decouple the collector name from registration. Other custom collector fields are also currently tied to registration. cc: @echlebek |
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.
Recommend that we re-evaluate this change based on this comment.
fguimond
left a comment
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
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, lets ensure that the tests pass and that the E2E tests test the upgrade scenario to ensure that collectors do not re-register on upgrade.
|
@crobert-1 would you be able to approve the pending workflows to run? Thank you! |
|
Scoped tests are failing, it appears to be related to this PR. Please fix the failing tests and I'll be able to review from there 👍 |
|
|
||
| _, err = credentialsStore.Get(hashKeyV2) | ||
| if err != nil { | ||
| logger.Info("credentials not found, trying legacy credentials", zap.Error(err)) |
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.
| logger.Info("credentials not found, trying legacy credentials", zap.Error(err)) | |
| logger.Info("credentials not found, using legacy credentials", zap.Error(err)) |
Nit: When I see the word trying I assume some other work will happen, but that's not the case. I think this wording may be more clear. Feel free to drop.
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Thank you for your contribution @pankaj101A! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Description
This pr removes the collector name from the local credentials path that extension uses for authentication. This allows changing the name of the collector without registration. See #42511
Changes
Link to tracking issue
Resolves #42511
Testing
Testing on local are performed link
Documentation