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.

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

Use the correct default for aws_auth.service for the AWS PRW exporter #3161

Merged
merged 3 commits into from
Apr 23, 2021
Merged

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Apr 17, 2021

"aps" is currently the only value that is acceptable for this field. We should have never provided it as a configuration setting but we can't remove and break the existing users. This change is setting it to "aps" by default.

A follow-up change will be provided to automatically parse the region from the workspace remote write endpoint.

@rakyll rakyll requested a review from a team as a code owner April 17, 2021 19:22
@codecov
Copy link

codecov bot commented Apr 17, 2021

Codecov Report

Merging #3161 (2a67ab0) into main (007b900) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
- Coverage   91.92%   91.91%   -0.02%     
==========================================
  Files         494      494              
  Lines       23962    23960       -2     
==========================================
- Hits        22028    22022       -6     
- Misses       1428     1430       +2     
- Partials      506      508       +2     
Flag Coverage Δ
integration 63.68% <ø> (-0.06%) ⬇️
unit 90.93% <66.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/awsprometheusremotewriteexporter/auth.go 82.60% <60.00%> (-4.06%) ⬇️
...porter/awsprometheusremotewriteexporter/factory.go 100.00% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 95.29% <0.00%> (-2.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 007b900...2a67ab0. Read the comment docs.

@rakyll
Copy link
Contributor Author

rakyll commented Apr 20, 2021

@anuraaga, can you take a look?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Think PR title and/or description need to be updated, one talks about region, other talks about service

@rakyll rakyll changed the title Use the correct default for aws_auth.region for the AWS PRW exporter Use the correct default for aws_auth.service for the AWS PRW exporter Apr 20, 2021
Copy link
Contributor Author

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

Thanks for the review, fixed the title.

@rakyll
Copy link
Contributor Author

rakyll commented Apr 21, 2021

@anuraaga is the test coverage a blocker here? We can do a follow up to improve code coverage in this package. The coverage results are not introduced by this change, they were historically low.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Don't think the coverage is a blocker. Thanks!

"aps" is currently the only value that is acceptable for this field. We should have never provided it as a configuration setting but we can't remove and break the existing users. This change is setting it to "aps" by default.

A follow up change will be provided to automatically parse the region from the workspace remote write endpoint.
@rakyll
Copy link
Contributor Author

rakyll commented Apr 22, 2021

@anuraaga Rebased this, now the build-push is passing. You can merge it.

@anuraaga
Copy link
Contributor

I can't merge but @bogdandrutu can :)

@bogdandrutu bogdandrutu merged commit 17399fa into open-telemetry:main Apr 23, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
…open-telemetry#3161)

* Use the correct default for aws_auth.region for the AWS PRW exporter

"aps" is currently the only value that is acceptable for this field. We should have never provided it as a configuration setting but we can't remove and break the existing users. This change is setting it to "aps" by default.

A follow up change will be provided to automatically parse the region from the workspace remote write endpoint.

* Reorder

* Improve godoc
mstumpfx pushed a commit to mstumpfx/opentelemetry-collector-contrib that referenced this pull request Aug 31, 2021
…open-telemetry#3161)

* Use the correct default for aws_auth.region for the AWS PRW exporter

"aps" is currently the only value that is acceptable for this field. We should have never provided it as a configuration setting but we can't remove and break the existing users. This change is setting it to "aps" by default.

A follow up change will be provided to automatically parse the region from the workspace remote write endpoint.

* Reorder

* Improve godoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants