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

If host.name value is present add it to resource labels #2709

Closed

Conversation

kisieland
Copy link
Contributor

Description:
I am proposing a change to resource translation (internal to OC).
It is motivated by the bug in stackdriverexporter, from the exporter perspective the host.name label is lost and the it is not exported by the pipeline.
I am proposing adding said label both to the node and resource labels.

Testing:
I've run unit tests for the changed library.
There seem to be so E2E failing tests, but I didn't debug them yet.

@kisieland kisieland requested a review from a team March 16, 2021 14:32
@bogdandrutu
Copy link
Member

bogdandrutu commented Mar 16, 2021

@kisieland why not fixing the stackdriver exporter to read the host.name from the ProcessIdentifier?

@kisieland
Copy link
Contributor Author

I will open PR to stackdriverexporter with the proposed fix.
I restrained from it as it might introduce quite a lot of code.

@kisieland kisieland closed this Mar 16, 2021
@bogdandrutu
Copy link
Member

@kisieland I can understand your point, but the translation logic does the right thing, and I would not add a hack in the core library just for one exporter bug :)

@jsuereth
Copy link
Contributor

Note: Let me know if we need to tweak the specification around resources in OC => OTel. If you're using OCA protocol to ingest into OpenTelemetry collector and resources are not mapped properly to OC semantic conventions, we may want to look at that in the context of https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/opencensus.md#resources

@kisieland
Copy link
Contributor Author

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.

3 participants