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

Map faas.name and faas.instance to prometheus job and instance for GMP #695

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 4, 2023

Step 1 for #679.

Since service.name has a default value, we need faas.name to take precedence over service.name. But I still want to give users the option to override job, so i've opted to add job as the override to match other labels.

For instance, i've opted to follow the pattern set by name.

In theory, this could be breaking for users if they are setting service.name or service.instance.id on cloud run and expect it to show up. But if we don't take this approach, this doesn't make the migration to the new value seamless for users that followed the example.

Thoughts? @punya @aabmass

@dashpole dashpole requested a review from a team as a code owner August 4, 2023 17:15
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #695 (c031790) into main (ce6e82d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #695   +/-   ##
=======================================
  Coverage   69.14%   69.14%           
=======================================
  Files          36       36           
  Lines        4728     4728           
=======================================
  Hits         3269     3269           
  Misses       1305     1305           
  Partials      154      154           
Files Changed Coverage Δ
...ector/googlemanagedprometheus/monitoredresource.go 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM and existing tests are passing

@dashpole dashpole merged commit b087585 into GoogleCloudPlatform:main Aug 7, 2023
@dashpole dashpole deleted the gmp_faas branch August 7, 2023 17:06
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.

2 participants