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

Add prometheus type field to EMF exporter for CloudWatch compatibility #2689

Merged
merged 1 commit into from
Mar 20, 2021

Conversation

bjrara
Copy link
Contributor

@bjrara bjrara commented Mar 14, 2021

Why do we need it?
Currently in CloudWatch agent, a field prom_metric_type will be added to CloudWatch log per item when metrics are collected from Prometheus. In order to keep the behavior the same as CloudWatch agent after user migrating to otel collector, we need to add the same field in EMF exporter.

@bjrara bjrara requested a review from anuraaga as a code owner March 14, 2021 17:27
@bjrara bjrara requested a review from a team March 14, 2021 17:27
@codecov
Copy link

codecov bot commented Mar 14, 2021

Codecov Report

Merging #2689 (74fd49a) into main (f8c550f) will increase coverage by 1.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2689      +/-   ##
==========================================
+ Coverage   90.53%   91.60%   +1.06%     
==========================================
  Files         444      445       +1     
  Lines       21984    22073      +89     
==========================================
+ Hits        19904    20220     +316     
+ Misses       1618     1384     -234     
- Partials      462      469       +7     
Flag Coverage Δ
integration 69.18% <ø> (?)
unit 90.53% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
exporter/awsemfexporter/metric_translator.go 97.91% <100.00%> (+0.20%) ⬆️
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️
internal/common/testing/container/container.go 73.68% <0.00%> (ø)
receiver/jmxreceiver/receiver.go 94.80% <0.00%> (+16.88%) ⬆️
receiver/dockerstatsreceiver/docker.go 92.30% <0.00%> (+39.05%) ⬆️
receiver/dockerstatsreceiver/receiver.go 96.72% <0.00%> (+47.54%) ⬆️
receiver/jmxreceiver/subprocess/subprocess.go 96.55% <0.00%> (+74.48%) ⬆️
receiver/redisreceiver/receiver.go 87.50% <0.00%> (+87.50%) ⬆️
receiver/redisreceiver/client.go 100.00% <0.00%> (+100.00%) ⬆️
receiver/redisreceiver/factory.go 100.00% <0.00%> (+100.00%) ⬆️
... and 1 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 f8c550f...74fd49a. Read the comment docs.

Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

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

Thanks!

@mxiamxia
Copy link
Member

@jrcamp , I carefully reviewed this PR and LGTM. Could you please help to review it?

kisieland pushed a commit to kisieland/opentelemetry-collector-contrib that referenced this pull request Mar 16, 2021
…2689)

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@mxiamxia
Copy link
Member

Hi @bogdandrutu , could you please help on this PR? Thanks!

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.

@mxiamxia I see a pattern of combining refactoring and logic changes into single PRs for this component. Can you help to make sure such changes are separated?

exporter/awsemfexporter/datapoint.go Outdated Show resolved Hide resolved
@anuraaga
Copy link
Contributor

Ah sorry I got confused it looks like this PR contains #2512 - so it means that's a dependency right?

@bjrara
Copy link
Contributor Author

bjrara commented Mar 18, 2021

@anuraaga Sorry about the confusion. I thought review in tandem would make it easier. Re-org the commit. Hope it help.

@bogdandrutu bogdandrutu merged commit 539932c into open-telemetry:main Mar 20, 2021
@bjrara bjrara deleted the prom_attr branch March 23, 2021 17:02
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
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.

5 participants