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

[ecs-metrics-receiver] extract cluster name from ARN and rename some constants #1626

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

hossain-rayhan
Copy link
Contributor

@hossain-rayhan hossain-rayhan commented Nov 18, 2020

Metadata received from Fargate gives us ClusterARN instead of ClusterName. So, This change extracts the ClusterName if we receive a ClusterARN. It utilizes our existing func (renamed it).

Also, renamed some resource names matching with semantic conventions. Once semantic convention for AWS PR is merged, we can easily change them without introducing any customer facing changes.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@@ -32,19 +32,19 @@ func containerResource(cm ContainerMetadata) pdata.Resource {

func taskResource(tm TaskMetadata) pdata.Resource {
resource := pdata.NewResource()
resource.Attributes().UpsertString(AttributeECSCluster, tm.Cluster)
resource.Attributes().UpsertString(AttributeECSCluster, getResourceFromARN(tm.Cluster))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't some unit tests need to be added / updated for this change?

Copy link
Contributor Author

@hossain-rayhan hossain-rayhan Nov 18, 2020

Choose a reason for hiding this comment

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

Yea, its using an existing function which I renamed. I modified existing unit tests to call this func and test possible cases- empty, correct, and incorrect value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new test for this. Thanks.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #1626 (b30fda5) into master (c01d3b0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1626      +/-   ##
==========================================
- Coverage   89.39%   89.39%   -0.01%     
==========================================
  Files         358      358              
  Lines       17741    17741              
==========================================
- Hits        15860    15859       -1     
- Misses       1399     1400       +1     
  Partials      482      482              
Flag Coverage Δ
integration 70.83% <ø> (-0.07%) ⬇️
unit 88.05% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...metricsreceiver/awsecscontainermetrics/resource.go 100.00% <100.00%> (ø)
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️

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 c01d3b0...b30fda5. Read the comment docs.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu @tigrannajaryan can we merge this please.

@tigrannajaryan tigrannajaryan merged commit 2398a00 into open-telemetry:master Nov 18, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
github was 503'ing on the URL (presumably because the diff is so large now). Don't bother checking the link.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* Set span status code, message and ref types according to the spec

* Serialize array attributes as string

* Use correct lib name / version key

* Add new and adjust existing tests

* Update CHANGELOG

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
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.

4 participants