Skip to content

Fix user agent strings and the otel uptime version label. #93

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
Merged
merged 5 commits into from
Jun 13, 2021
Merged

Fix user agent strings and the otel uptime version label. #93

merged 5 commits into from
Jun 13, 2021

Conversation

@igorpeshansky
Copy link
Contributor

@igorpeshansky igorpeshansky commented Jun 9, 2021 •
edited
Loading

Update tests to exercise otel on linux.
Also synchronize error behavior between otel and collectd, and some cleanup of test inputs.

qingling128
qingling128 reviewed Jun 9, 2021
View reviewed changes
confgenerator/confgenerator.go Outdated
Copy link
Contributor

@qingling128 qingling128 Jun 9, 2021

Choose a reason for hiding this comment

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

The getVersionLabel and getUserAgent functions are quite similar. Does it make sense to combine them?

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jun 9, 2021

Choose a reason for hiding this comment

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

Good idea. Done. As an aside, this also made me realize that getVersionLabel does not use the hostInfo parameter, so I've removed that.

confgenerator/testdata/invalid/linux/metrics-receiver_unsupported_type/golden_error Outdated
Copy link
Contributor

@qingling128 qingling128 Jun 9, 2021

Choose a reason for hiding this comment

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

On Linux, iis and mssql are not supported, right?

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jun 9, 2021

Choose a reason for hiding this comment

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

Correct, but this is effectively the TODO you've previously added in confgenerator.go, except for otel. I've replicated that TODO to all of the affected locations.

Copy link
Contributor

@qingling128 qingling128 Jun 12, 2021

Choose a reason for hiding this comment

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

Ah I see. This changed because we switched from collectd to otel, which has a TODO. Got it.

otel/conf.go Outdated
Copy link
Contributor

@qingling128 qingling128 Jun 9, 2021

Choose a reason for hiding this comment

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

Nice catch! Is this worth a separate PR though? Seems a bit unrelated to Fix user agent strings and the otel uptime version label., right?

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jun 9, 2021

Choose a reason for hiding this comment

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

Ah. This is part of synchronizing the error messages across collectd and otel… I'm happy to mention that in the PR description if needed.

@igorpeshansky
Copy link
Contributor Author

igorpeshansky commented Jun 9, 2021

As agreed offline, I've split out the first two commits of this PR into #94, and will rebase once that one is merged.

@qingling128
Copy link
Contributor

qingling128 commented Jun 10, 2021

#94 was merged. Guess we could rebase this one then. Let me know when it's ready for review.

@igorpeshansky igorpeshansky force-pushed the igorpeshansky-otel-useragent branch 2 times, most recently from 5734136 to c262a21 Compare June 11, 2021 23:16
@igorpeshansky
Copy link
Contributor Author

igorpeshansky commented Jun 12, 2021

Rebased. Ready for review.

qingling128
qingling128 approved these changes Jun 12, 2021
View reviewed changes
Copy link
Contributor

@qingling128 qingling128 left a comment

Choose a reason for hiding this comment

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

LGTM

confgenerator/testdata/invalid/linux/metrics-receiver_unsupported_type/golden_error Outdated
Copy link
Contributor

@qingling128 qingling128 Jun 12, 2021

Choose a reason for hiding this comment

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

Ah I see. This changed because we switched from collectd to otel, which has a TODO. Got it.

@igorpeshansky igorpeshansky merged commit ab1eb82 into master Jun 13, 2021
@igorpeshansky igorpeshansky deleted the igorpeshansky-otel-useragent branch June 13, 2021 02:18
qingling128 added a commit that referenced this pull request Jun 17, 2021
@qingling128
Partially cherry pick: Fix user agent strings and the otel uptime ver…
0d3963d 
…sion label. (#93)

Only cherry pick the commit ab1eb82 with the
 part that updates user agent and version label.
qingling128 added a commit that referenced this pull request Jun 18, 2021
@qingling128
Partially cherry pick: Fix user agent strings and the otel uptime ver…
6a4c06b 
…sion label. (#93) (#106)

Only cherry pick the commit ab1eb82 with the part that updates user agent and version label.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qingling128 qingling128 qingling128 approved these changes

@quentinmit quentinmit Awaiting requested review from quentinmit

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@igorpeshansky @qingling128