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

Add a second argument to callbacks for grpc streams with metadata #3801

Conversation

cchamplin
Copy link
Contributor

What?

Change updates the grpc stream callbacks to all receivers to be passed a meta data object that includes the timestamp of the original event. This change facilitates being able to accurately check the time when a message was received by a client stream.

Why?

During during load testing it may be important to know exactly when specific messages are received by the client to track various heuristics. At the moment this is difficult or impossible to achieve because the callbacks are only executed when the vu's tick, so there may be a floor of the vu internal for all measured timestamps on messages unless the time the message was received is captured by the k6 runtime and passed into the vu.

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)

No issue exists yet

@cchamplin cchamplin requested a review from a team as a code owner June 19, 2024 01:58
@cchamplin cchamplin requested review from codebien and joanlopez and removed request for a team June 19, 2024 01:58
@codebien
Copy link
Contributor

codebien commented Jul 1, 2024

Hey @cchamplin,

During during load testing it may be important to know exactly when specific messages are received by the client to track various heuristics.

Can you expand more on your use case? What kind of logic do you want to apply based on each single stream's request? Did you try to implement the measurement directly in your JavaScript code?

The current proposed solution is very opinionated and specific for your case, it is worth exploring if we can do something a bit more generic.

Metadata is a very specific concept in the gRPC ecosystem, did you pick meta on purpose for the argument name?

@cchamplin
Copy link
Contributor Author

@codebien We have a metric we are trying to capture that is essentially the around trip time it takes for certain events to arrive at a grpc client. At the moment we have not found a way to accurately capture this since we don't want to use a timestamp from the server and if we grab the data/time in the callback for the grpc stream message that may off by minIterationDuration or any amount of time the VU has slept. So we're looking for a way to add some metadata (general sense) around when exactly a message was received that will not be impacted by VU's sleeping. We're open to any solutions for this.

@codebien
Copy link
Contributor

codebien commented Jul 2, 2024

Hey, thanks for sharing. I thought a bit more about this and it makes sense. In the end, this is what we do with Response.timings on the HTTP module.

Let me sync internally regarding the specific API, I would prefer if we have something consistent between modules and gRPC functions (both unary and streaming).

@codebien
Copy link
Contributor

codebien commented Aug 21, 2024

Hey @cchamplin, the team synced on this pull request and we are fine with the current proposal. Please, can you rebase so we can merge it as-is?

@codebien
Copy link
Contributor

Hey @joanlopez, I won't be able to rebase this, can you take care of it, please?

@codebien codebien requested review from a team and olegbespalov and removed request for codebien and a team August 30, 2024 15:02
Change updates the grpc stream callbacks to all receivers to be passed
a meta data object that includes the timestamp of the original event.
This change facilitates being able to accurately check the time when a
message was recevied by a client stream.
@cchamplin cchamplin force-pushed the feature/add-timestamp-to-grpc-event-callbacks branch from a474555 to 08a5066 Compare August 30, 2024 15:55
@olegbespalov olegbespalov added documentation-needed A PR which will need a separate PR for documentation area: grpc labels Sep 3, 2024
@olegbespalov olegbespalov added this to the v0.54.0 milestone Sep 3, 2024
@olegbespalov
Copy link
Contributor

@joanlopez I believe you also have some chats about the PR with codebien, so I'm positioning the final approve and merging this one with you

@olegbespalov
Copy link
Contributor

@cchamplin, thanks for the contribution!

@joanlopez joanlopez merged commit f53d1c6 into grafana:master Sep 12, 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 documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants