Fixed Summary Generated Events

[ameetpal]

What?

This PR fixes the shortcomings of TestSummaryGenerated Event :- https://github.com/grafana/k6/pull/3682

Why?

The previously added TestSummaryGenerated had data in the form of a map with keys of type string and values of type io.Reader, however, the stream had already been read by stdout. Additionally, the data was formatted for console output, which was not consistent with what was expected by the handleSummary function of the test. Therefore, I returned the exact summary passed into the function and sent it as an event.

Checklist

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

Related PR(s)/Issue(s)

[olegbespalov]

Hey @ameetpal. After an internal discussion, we decided to revert to the original changes to reduce it from the scope of the upcoming release #3736 (since they will change). In the next v0.51, we could continue working on that PR. In any case, thank you for your contributions!

[olegbespalov]

Hey @ameetpal

We did release the k6 v0.51. Could you please rebase this PR using the latest master branch? Also, out of curiosity, this version of the changes satisfies your needs, right?

[ameetpal]

Hey @olegbespalov , i have updated the branch, some random tests are failing, but are not related to my changes Yes these changes are correct

[ameetpal]

@codebien can you please provide your review

[ameetpal]

@codebien I have added a test for jsSummary. Also i have documented HandleSummary function but i have not added detailed keys and values as it may change based on a test and also boader key can change in future in runner function which will result in stale comment. If you still want, i can add comment to current keys that are being returned.

[olegbespalov]

@ameetpal, out of curiosity, do the changes from this PR cover your case from #3732? I mean, have you tested everything assembled? I'm asking since we'd like to be sure that this PR serves the real needs

[codebien]

@ameetpal to exapand a bit @olegbespalov's request. Can you also clarify if this pull request is going to solve https://github.com/grafana/k6/issues/3732 for you?

Can you also explain a bit more about how you intend to use it, please? So, in the case we decide to document this feature, we could use your use case as an example in the documentation. Or, we can have it here just as historical information in case someone in the future needs to investigate more on why we decided to introduce this event.

[ameetpal]

Hey @olegbespalov , @codebien , as of now i will consume this event in the new extension that i am working on. I was thinking of some way to include extension without explicitly importing this in test script but since this is not an option now, we will ask user to import our extension in the test script. I have tested this in local, and that is why i have raised this PR, since i was getting empty data in extension from the changes in my previous PR. Currently this is the only feasible method we find to solve our problem.

[ameetpal]

Also, if we are able to use this successfuly, in future we might flow out other events like TestStarted. For auditing purposes.

[ameetpal]

@codebien any updates here ?

[codebien]

Hi @ameetpal, I and @olegbespalov met and synced on this pull request.

The current approach doesn't seem to be ideal, because it creates a dependency with an internal API. The current submitted changes rely upon a map[] structure designed internally to the js package. It mostly prepares the structure to serialize the Summary as a JSON. It is used from --summary-export feature, which is already a deprecated part of k6 that we might change in the future.

Instead, a more solid option could be to export lib.Summary type as Data field for the event emitted. In that way, you can create directly in your extension the best structure for your use case. lib.Summary is the type currently passed through HandleSummary and it is already under a shared package (lib). https://github.com/grafana/k6/blob/f57dc2f76826d3601e1c4b28039be373849fc4f3/cmd/run.go#L195

Does this proposal still solve your problem?

[joanlopez]

Moving to https://github.com/grafana/k6/milestone/42 since there's no news since a while.

[codebien]

Hey @ameetpal, do you have any news on the suggested approach, and would it work for you?

[ameetpal]

Hey @codebien , approch seems good, i hot busy with some other task, will push changes in 2 or 3 days

[ameetpal]

Hey @codebien, i have incorporated the comment, please review the PR

[codebien]

Hey @ameetpal, We have discussed it internally and unfortunately, we decided to not encourage you to continue the development, because we might not be able to merge soon the pull request.

As the previous comments demonstrate, there is uncertainty and unknown around this part of the code. The feature requires detailed discussions and knowledge of the k6 codebase. It has implications across the entire tool including risks, and on the other hand, it has no direct benefit for k6 users. Furthermore, the k6 end of test summary will be under refactoring in the upcoming releases, and there are high chance that we might end up breaking change this API. As well as the anticipated experimental Event’s API.

Those reasons convinced us that we shouldn’t pursue the current proposal, because it might slow down future development and generate unexpected events.

The correct way for you to extend this functionality is by having a custom output extension to implement your custom summary and related process.

I apologize if we weren't able to communicate this at the beginning, but I hope you can understand that it required research and discussions.

[ameetpal]

Closing this PR