k6 Insights (1/2): Refactor request metadata output to a separate package #3201
Conversation
Blinkuu
force-pushed
the
refactor_request_metadata_output_to_separate_package
branch
from
a0d553b to
47c78a5
Compare
Codecov Report
@@ Coverage Diff @@
## master #3201 +/- ##
==========================================
+ Coverage 72.85% 72.87% +0.01%
==========================================
Files 256 259 +3
Lines 19804 19810 +6
==========================================
+ Hits 14429 14436 +7
+ Misses 4474 4473 -1
Partials 901 901
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
It generally looks good. Left two minor comments 👍
Also, adding the @oleiade as the second reviewer from the core.
| @@ -156,14 +150,14 @@ func (o *Output) Start() error { | |||
| } | |||
| insightsClient := insights.NewClient(insightsClientConfig) | |||
|
|
|||
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) | |||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |||
There was a problem hiding this comment.
Why has this increased ten times? 🤔
If we are not sure about the value, it seems better to make this configurable
There was a problem hiding this comment.
I have increased this since 1 second to establish a connection is not that long, considering that our backend is in a single region, but the cloud tests can be distributed geographically. This can be inferred from RTT times across the globe.
There was a problem hiding this comment.
Okay, it still sounds like we should have these settings configurable to have a chance to adjust them quickly, along with some from the
k6/output/cloud/expv2/output.go
Lines 128 to 151 in 47c78a5
But it could be a subject of the separated PR 👍
There was a problem hiding this comment.
Ideally, yeah. However, these are already tuned based on a few experiments we did in the past. Let's leave this for the future.
olegbespalov
requested review from
oleiade
and removed request for
mstoykov and
codebien
What?
This PR refactors the request metadata logic into a separate package so we can later reuse it in cloud output v1.
Why?
Due to cloud output v2 being at risk for v0.46.0, we decided to incorporate the new request metadata output into cloud output v1. This change is done to mitigate the risk of failing to deliver the new insights features.
Related PR(s)/Issue(s)
This PR is part of a PR chain. Next PR - #3202.
Previous PRs: