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

Fix metrics for GRPC streams do not correctly include tags when stream ends #3746

Merged

Conversation

cchamplin
Copy link
Contributor

@cchamplin cchamplin commented May 17, 2024

What?

Update the grpc stream being call to pass tags and params that include custom tags and method information

Why?

Addresses #3745

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

#3745

Closes #3745

@cchamplin cchamplin requested a review from a team as a code owner May 17, 2024 03:27
@cchamplin cchamplin requested review from oleiade and olegbespalov and removed request for a team May 17, 2024 03:27
@CLAassistant
Copy link

CLAassistant commented May 17, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Hi @cchamplin

Good catch! Thank you for your contribution!

The change looks good to me, but it is also essential to introduce the test for it. It could be similar to something from the cmd_run_grpc_test.go, and the getSampleValues could be used to extract metrics/tags.

Do you think you could address this, or should we consider splitting a test into a separate issue or pull request?

@olegbespalov olegbespalov added awaiting user waiting for user to respond area: grpc labels May 23, 2024
@olegbespalov olegbespalov added this to the v0.52.0 milestone May 23, 2024
@olegbespalov
Copy link
Contributor

@cchamplin just double-checking if you don't have the capacity for the tests? I could take over; one thing is, could you please maybe name the case? I mean which metrics missed the custom tags? Thanks!

@joanlopez
Copy link
Contributor

Pushing to https://github.com/grafana/k6/milestone/42 while waiting for news.

@joanlopez joanlopez modified the milestones: v0.52.0, v0.53.0 Jun 18, 2024
@cchamplin cchamplin force-pushed the bugfix/grpc-stream-error-tags branch from fa8e9e2 to 75be2ca Compare June 19, 2024 01:24
@cchamplin
Copy link
Contributor Author

@olegbespalov @joanlopez I have added the test as requested, let me know if that works.

@cchamplin cchamplin force-pushed the bugfix/grpc-stream-error-tags branch from 75be2ca to 6886cba Compare June 19, 2024 12:39
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

@cchamplin great, thanks! I left the last comment about simplifying the logic. Apart from that, it seems green from my side 👍

@oleiade oleiade removed their request for review June 26, 2024 13:17
@joanlopez
Copy link
Contributor

Hey @cchamplin,
Could you try to address what @olegbespalov suggested, please?
Once done, I think we'll be ready to merge it. Thanks! 🙇🏻

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Okay, we could merge this as it's and update the test in a separate PR. Thanks for the contribution!

@olegbespalov olegbespalov merged commit e79ee2c into grafana:master Jul 15, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: grpc awaiting user waiting for user to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics for GRPC streams do not correctly include tags when stream ends.
4 participants