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

Rename [Traces|Metrics|Logs]Consumer to [Traces|Metrics|Logs] #2761

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Mar 22, 2021

Updates #2759

Signed-off-by: Bogdan Drutu [email protected]

@bogdandrutu bogdandrutu requested a review from a team March 22, 2021 22:31
@bogdandrutu
Copy link
Member Author

/cc @Aneurysm9

@bogdandrutu bogdandrutu changed the title Renam [Traces|Metrics|Logs]Consumer to [Traces|Metrics|Logs] Rename [Traces|Metrics|Logs]Consumer to [Traces|Metrics|Logs] Mar 22, 2021
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #2761 (364a287) into main (3660f5c) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2761   +/-   ##
=======================================
  Coverage   91.83%   91.83%           
=======================================
  Files         291      291           
  Lines       15522    15524    +2     
=======================================
+ Hits        14254    14256    +2     
  Misses        867      867           
  Partials      401      401           
Impacted Files Coverage Δ
component/componenttest/nop_exporter.go 100.00% <ø> (ø)
component/componenttest/nop_processor.go 100.00% <ø> (ø)
component/componenttest/nop_receiver.go 100.00% <ø> (ø)
consumer/consumertest/err.go 100.00% <ø> (ø)
consumer/consumertest/nop.go 100.00% <ø> (ø)
consumer/consumertest/sink.go 100.00% <ø> (ø)
consumer/fanoutconsumer/cloningconsumer.go 57.57% <ø> (ø)
exporter/exporterhelper/factory.go 100.00% <ø> (ø)
internal/testcomponents/example_exporter.go 100.00% <ø> (ø)
processor/attributesprocessor/factory.go 82.60% <ø> (ø)
... and 40 more

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 3660f5c...364a287. Read the comment docs.

@@ -593,7 +593,7 @@ func (pds *PrometheusDataSender) Start() error {
return err
}

pds.MetricsConsumer = exp
pds.Metrics = exp
Copy link
Member

Choose a reason for hiding this comment

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

This is worse than before. It is not clear what this Metrics is since it is a very generic word. I do not mind renaming the type names but if we do that we must ensure embedded structs become fields so that they have a proper name otherwise readability of the code suffers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree that embedded fields are not best named in this situation, I would still do the change because it is more go standard way to name things like this. Do you mind if I look into this in a followup?

internal/testcomponents/example_exporter.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit 0c6757e into open-telemetry:main Mar 23, 2021
@bogdandrutu bogdandrutu deleted the consumer branch February 1, 2022 02:32
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

2 participants