-
Notifications
You must be signed in to change notification settings - Fork 2
chore: rename NRDOT agent type #1745
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
chore: rename NRDOT agent type #1745
Conversation
alvarocabanas
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
5ceecbc to
1a549d7
Compare
Remaining places: - [ ] `helm-charts` repo - [ ] `docs-website` - [ ] `beta-docs-site`
1a549d7 to
d5a09e4
Compare
|
Could you update the PR description and briefly why we keep both agent-types? |
sigilioso
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.
Thanks! 🚀
Maybe a comment in the old agent-type saying that it is kept for backwards compatibility reasons might be useful for future us.
| backoff_strategy: | ||
| type: fixed | ||
| backoff_delay: ${nr-var:backoff_delay} | ||
| # See com.newrelic.infrastructure Agent type for description of fields. |
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.
I've just noticed this comment, copy&pasted for sure but it is a bit misleading, isn't it?
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.
Well the com.newrelic.infrastructure has some descriptions for the fields in comments, so perhaps it makes sense to leave this?
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.
Oh, now I get it. Let's leave it for now but it is really strange: we define the description field in the com.newrelic.infrastructure Agent Type (those fields could be actually used!) and here we add a comment asking to check the other Agent Type 🙃
| "agents": { | ||
| "nrdot":{ | ||
| "agent_type" : "newrelic/com.newrelic.opentelemetry.collector:0.1.0", | ||
| "agent_type" : "newrelic/io.opentelemetry.collector:0.1.0", |
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.
We can change it again after we release, right?
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.
Yup! But I'm not entirely sure why it's faling... can we review it?
Agent types `io.opentelemetry.collector` and `com.newrelic.opentelemetry.collector` should be the same except for the name
Added! Thanks for the heads up. |
What this PR does / why we need it
This PR renames* the agent type for the New Relic Distribution for the OTel Collector (NRDOT). Now it's called
com.newrelic.opentelemetry.collector.Special notes for your reviewer
(* For compatibility reasons, as there will be configurations and deployments using the old agent type metadata, we now maintain both the old agent type name and the new, being equal in all respects besides the name. Fleet Control team also supports both so there should be no problems with new configs supporting the newer agent type.)
Remaining places to rename:
helm-chartsrepo: [agent-control-deployment] chore: rename NRDOT agent type helm-charts#1952docs-website: chore: rename NRDOT agent type docs-website#21842beta-docs-site: https://github.com/newrelic/beta-docs-site/pull/577Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
docsis aligned with the change.CONTRIBUTING.md.log level guidelines.