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

feat: catch, discard & warn about the labels that have reserved names #3162

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Jul 4, 2023

What?

Cloud Output V2 drops labels that are reserved, for now, we're sticking with the test_run_id and Prometheus system metrics (everything that prefixed with __)

Why?

These tags are reserved for internal use and can't be set with the client.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3155

@olegbespalov olegbespalov self-assigned this Jul 4, 2023
@github-actions github-actions bot requested review from codebien and oleiade July 4, 2023 13:19
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

It requires minimizing the allocation because we invoke this code with high frequency.

output/cloud/expv2/mapping.go Outdated Show resolved Hide resolved
output/cloud/expv2/flush.go Outdated Show resolved Hide resolved
@olegbespalov olegbespalov force-pushed the feat/cloudv2-discard-reserved-metrics branch from 6fc9674 to 66372db Compare July 5, 2023 09:35
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2023

Codecov Report

Merging #3162 (767fba9) into master (aa79387) will increase coverage by 0.05%.
The diff coverage is 90.90%.

❗ Current head 767fba9 differs from pull request most recent head 327ab5a. Consider uploading reports for the commit 327ab5a to get more accurate results

@@            Coverage Diff             @@
##           master    #3162      +/-   ##
==========================================
+ Coverage   72.78%   72.84%   +0.05%     
==========================================
  Files         255      255              
  Lines       19585    19611      +26     
==========================================
+ Hits        14255    14285      +30     
+ Misses       4433     4430       -3     
+ Partials      897      896       -1     
Flag Coverage Δ
ubuntu 72.78% <90.90%> (+0.05%) ⬆️
windows 72.67% <90.90%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/expv2/flush.go 90.16% <89.47%> (+3.20%) ⬆️
output/cloud/expv2/mapping.go 94.80% <91.66%> (+0.68%) ⬆️
output/cloud/expv2/output.go 66.45% <100.00%> (+0.43%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM 🙇 Just a minor on the warn message.

output/cloud/expv2/flush.go Outdated Show resolved Hide resolved
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Just caught only now. We can just use a slice storing the tags' keys during the detection process. A TagSet shouldn't include multiple tags with the same key.

output/cloud/expv2/mapping.go Outdated Show resolved Hide resolved
output/cloud/expv2/flush.go Outdated Show resolved Hide resolved
output/cloud/expv2/flush.go Show resolved Hide resolved
Base automatically changed from cloudv2-proto-common-fields to master July 5, 2023 12:07
@olegbespalov olegbespalov force-pushed the feat/cloudv2-discard-reserved-metrics branch from 69cf43a to 12e95f6 Compare July 5, 2023 12:12
@olegbespalov olegbespalov force-pushed the feat/cloudv2-discard-reserved-metrics branch 2 times, most recently from a786d21 to 5941821 Compare July 5, 2023 12:17
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@olegbespalov olegbespalov force-pushed the feat/cloudv2-discard-reserved-metrics branch from 5941821 to 327ab5a Compare July 5, 2023 12:26
@olegbespalov olegbespalov added this to the v0.46.0 milestone Jul 5, 2023
@olegbespalov olegbespalov requested a review from esquonk July 5, 2023 14:27
@olegbespalov
Copy link
Contributor Author

@esquonk, adding you also as the reviewer, probably the most interesting part for you is https://github.com/grafana/k6/pull/3162/files#diff-2edc8a9a0a0d4ec2a88767baae9d8ad5c035168eeee41b7b5c8b36ce31d9ce70R42-R49

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

👏🏻 🙇🏻

output/cloud/expv2/mapping.go Show resolved Hide resolved
@olegbespalov olegbespalov merged commit 0fb962b into master Jul 6, 2023
@olegbespalov olegbespalov deleted the feat/cloudv2-discard-reserved-metrics branch July 6, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn and discard any label equal to the reserved
4 participants