Fixed Summary Generated Events #3730
Conversation
ameetpal
requested review from
codebien and
olegbespalov
and removed request for
a team
There was a problem hiding this comment.
LGTM
@codebien and maybe @grafana/k6-core we might consider adding this into upcoming release
|
Hey @ameetpal We did release the k6 v0.51. Could you please rebase this PR using the latest |
|
Hey @olegbespalov , i have updated the branch, some random tests are failing, but are not related to my changes |
|
@codebien can you please provide your review |
|
@codebien I have added a test for jsSummary. |
…est-Summary-generated
|
@ameetpal to exapand a bit @olegbespalov's request. Can you also clarify if this pull request is going to solve #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. |
|
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. |
|
Also, if we are able to use this successfuly, in future we might flow out other events like TestStarted. For auditing purposes. |
js/summary_test.go
Outdated
| require.NotNil(t, result["rawdata.json"]) | ||
| newRawData, err := io.ReadAll(result["rawdata.json"]) | ||
| require.NoError(t, err) | ||
| assert.JSONEq(t, string(newRawData), string(jsonString)) |
There was a problem hiding this comment.
Hi @ameetpal,
thanks again for your contribution! 🙇
As you are asserting here, they are mostly the same. So, what is the reason for having them in different variables? Can you explain better it, please?
Ideally, we should try to not change the Runner interface, because it is an abstraction and it is a very central component.
If the current map[string]io.Reader is not the ideal type and we need to change it, we can create a function in js package that accepts this type and returns the type you need. Or, probably, even better to do it directly where we are generating the event so in cmd package.
There was a problem hiding this comment.
@codebien
Both are same in test as i want to get the data supplied to handleSummary function in test script file.
map[string]io.Reader, contains stdout related data which can only be read once. Also we can use this read also, but we need to write a converter function that will return the json data from which it was created.
If you want i can look into that approach also
|
@codebien any updates here ? |
|
Hi @ameetpal, 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 Instead, a more solid option could be to export Line 195 in f57dc2f Does this proposal still solve your problem? |
|
Moving to https://github.com/grafana/k6/milestone/42 since there's no news since a while. |
|
Hey @ameetpal, do you have any news on the suggested approach, and would it work for you? |
|
Hey @codebien , approch seems good, i hot busy with some other task, will push changes in 2 or 3 days |
…est-Summary-generated
…k6 into feature/Test-Summary-generated
|
Hey @codebien, i have incorporated the comment, please review the PR |
| if hsErr == nil { | ||
| hsErr = handleSummaryResult(c.gs.FS, c.gs.Stdout, c.gs.Stderr, summaryResult) | ||
| waitForSummaryGeneratedEvent := emitEvent(&event.Event{ | ||
| Type: event.TestSummaryGenerated, | ||
| Data: summary, |
There was a problem hiding this comment.
It should be better to copy ObservedMetrics and RootGroup instead of directly passing them. They are pointers and they might generate data races if there is a read/write across extensions and/or from k6core.
| func (s *Summary) String() string { | ||
| return "&{Summary:map[...]...}" | ||
| } |
There was a problem hiding this comment.
I guess when we agree on the generation part then we should update this with some additional information. We might have Summary{Metrics: number, RootGroup: name/(or nil), and the other fields}.
|
Hey @ameetpal, 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. 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. |
|
Closing this PR |
What?
This PR fixes the shortcomings of TestSummaryGenerated Event :- #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
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)