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

Populate resource to OTLP proto data #758

Merged
merged 6 commits into from
May 16, 2021

Conversation

ThomsonTan
Copy link
Contributor

@ThomsonTan ThomsonTan commented May 15, 2021

Fixes #757

Changes

Populate resource to OTLP proto data and also update the readme for OTLP receiver with easier instructions to run and test it end-to-end with docker.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@ThomsonTan ThomsonTan requested a review from a team May 15, 2021 03:48
@codecov
Copy link

codecov bot commented May 15, 2021

Codecov Report

Merging #758 (bb95dd1) into main (3bfbc93) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #758      +/-   ##
==========================================
+ Coverage   95.94%   95.95%   +0.01%     
==========================================
  Files         174      174              
  Lines        7127     7127              
==========================================
+ Hits         6838     6839       +1     
+ Misses        289      288       -1     
Impacted Files Coverage Δ
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <0.00%> (+2.12%) ⬆️


rec->PopulateProtoResource(proto_resource.get());

resource_span->set_allocated_resource(proto_resource.release());
Copy link
Member

Choose a reason for hiding this comment

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

nit - Just wondering if line 35 - 40 can be replaced with single line:

resource_span->mutable_resource() = rec->ProtoResource();

using older implementation of ProtoResource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose set_allocated_resource which could avoid allocate the Resource twice or more, but yes, it look simpler with mutable_resource. Updated the PR to the older implementation of ProtoResource as this optimization may not deserve the extra complexity.

Copy link
Member

@lalitb lalitb 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 adding the support. Looks good, perhaps we can add unit tests for the resources too.

@ThomsonTan
Copy link
Contributor Author

Thanks for adding the support. Looks good, perhaps we can add unit tests for the resources too.

Added unit resource test to OTLP recordable test case. We may need more tests if we'd like to validate the OTLP exported result.

@lalitb lalitb merged commit d83e4b6 into open-telemetry:main May 16, 2021
@ThomsonTan ThomsonTan deleted the PopulateResourceToOtlp branch November 9, 2022 22:57
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.

Resource is not populated to OTLP proto data
2 participants