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

Switch from detecting faas.id to faas.instance in the gcp detector. #26486

Merged
merged 3 commits into from
Oct 6, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Sep 6, 2023

Description:

Context: GoogleCloudPlatform/opentelemetry-operations-go#679

faas.id was removed from the semantic conventions: open-telemetry/opentelemetry-specification#3188

We were previously using it improperly to store the instance id of the faas, which should be mapped to faas.instance instead.

Testing:

Unit tests

@dashpole dashpole requested a review from a team September 6, 2023 18:59
@github-actions github-actions bot added the processor/resourcedetection Resource detection processor label Sep 6, 2023
@dashpole dashpole added the comp:google Google Cloud components label Sep 6, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I believe since this is an end-user impacting breaking change it should be done via feature-gates https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#end-user-impacting-changes

@dashpole
Copy link
Contributor Author

@codeboten I added a feature gate, and changed the PR from breaking to deprecation.

@dashpole dashpole force-pushed the update_resource_detector branch 2 times, most recently from 419fff0 to 12417ee Compare September 29, 2023 16:13
@dashpole
Copy link
Contributor Author

@codeboten @dmitryax @Aneurysm9 PTAL if you have time.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @dashpole, just one suggestion

processor/resourcedetectionprocessor/internal/gcp/gcp.go Outdated Show resolved Hide resolved
@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Oct 4, 2023
@codeboten
Copy link
Contributor

Please resolve the conflict

@dashpole
Copy link
Contributor Author

dashpole commented Oct 5, 2023

@codeboten conflict resolved.

Co-authored-by: Alex Boten <aboten@lightstep.com>
@dashpole
Copy link
Contributor Author

dashpole commented Oct 6, 2023

@codeboten sorry. Forgot to pull in origin when I fixed go.mod

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

np! thanks for following up

@codeboten codeboten merged commit f9a1a52 into open-telemetry:main Oct 6, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 6, 2023
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
…pen-telemetry#26486)

Context:
GoogleCloudPlatform/opentelemetry-operations-go#679

faas.id was removed from the semantic conventions:
open-telemetry/opentelemetry-specification#3188

We were previously using it improperly to store the instance id of the
faas, which should be mapped to faas.instance instead.

---------

Co-authored-by: Alex Boten <aboten@lightstep.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:google Google Cloud components processor/resourcedetection Resource detection processor ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants