output/cloudv2: Implement RequestMetadata output #3100

Blinkuu · 2023-05-29T15:33:45Z

This PR adds support for sending request metadata to the k6 Insights cloud backend.

codecov-commenter · 2023-05-31T12:32:49Z

Codecov Report

Merging #3100 (1c2fd65) into master (9fdd569) will decrease coverage by 1.87%.
The diff coverage is 33.94%.

❗ Current head 1c2fd65 differs from pull request most recent head 6ffe102. Consider uploading reports for the commit 6ffe102 to get more accurate results

@@            Coverage Diff             @@
##           master    #3100      +/-   ##
==========================================
- Coverage   73.88%   72.01%   -1.87%     
==========================================
  Files         243      252       +9     
  Lines       18488    19405     +917     
==========================================
+ Hits        13660    13975     +315     
- Misses       3961     4537     +576     
- Partials      867      893      +26

Flag	Coverage Δ
ubuntu	`71.95% <33.94%> (-1.86%)`	⬇️
windows	`71.84% <33.94%> (-1.88%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
output/cloud/expv2/output.go	`66.01% <5.12%> (-20.83%)`	⬇️
cloudapi/insights/proto/v1/common/common.pb.go	`15.20% <15.20%> (ø)`
cloudapi/insights/proto/v1/ingester/ingester.pb.go	`22.99% <22.99%> (ø)`
cloudapi/insights/proto/v1/trace/span.pb.go	`25.80% <25.80%> (ø)`
...api/insights/proto/v1/ingester/ingester_grpc.pb.go	`27.02% <27.02%> (ø)`
cloudapi/insights/proto/v1/k6/labels.pb.go	`27.47% <27.47%> (ø)`
cloudapi/insights/proto/v1/trace/labels.pb.go	`32.25% <32.25%> (ø)`
...oudapi/insights/proto/v1/k6/request_metadata.pb.go	`32.85% <32.85%> (ø)`
cloudapi/insights/client.go	`68.11% <68.11%> (ø)`
cloudapi/insights/mappers.go	`82.85% <82.85%> (ø)`
... and 3 more

... and 3 files with indirect coverage changes

Blinkuu · 2023-05-31T12:45:36Z

output/cloud/expv2/output.go

+	// TODO(lukasz): Check if the `tracing` module is imported.
+	//
+	// We want to check if the `tracing` module was imported. If
+	// it wasn't, we don't need to collect any data related to tracing
+	// as there won't be any.
+	//
+	// We currently don't have a way to check if the tracing module
+	// was imported by the user.


I want to discuss addressing this matter.

Ideally, we would like to enable the request metadata output logic if all 3 conditions are met:

A user (or robot user) sets the K6_CLOUD_TRACES_ENABLED flag to true.

The executed script has imported the experimental/tracing module.

A user is eligible for the k6 x Tempo integration (aka. we know they are both Grafana k6 Cloud and Grafana Cloud Traces customers).

Regarding (1), this is already being verified.

Regarding (2), we have to figure out whether it's possible to implement in the project's current state and how "clean" or "hacky" the solution would be. From my analysis, it seems we could hack around this with a global variable (😅) set in the js/modules/k6/experimental/tracing package once a module is instantiated (imported by a user). Otherwise, we would somehow have to introduce a dependency on the tracing module in this v2 output package. Anyhow, let me know your thoughts.

Regarding (3), we still have to implement this on the backend side. We are currently not prioritizing it for the 1.0 release, so the idea is to introduce it in the future. Even without this check, ingesting the data is safe, and we will handle dropping it on the backend side.

My initial thought is that (2) is not actually a requirement at all.

FIrst and foremost just because someone imports this doesn't mean that they are not or they are using it to do tracing. Especially on the not side.

In this version of k6 #3037 will be released which means that practically the whole k6/experimental/tracing can now be written entirely in js code.

It also means that users can now create and tag metrics with metadata that looks like the one from k6/experimental/tracing. And they might want to do it for other protocols/APIS (including not HTTP ones, which I guess we are not going to support initially).

Given that and the fact that K6_CLOUD_TRACES_ENABLED flag I feel like this is needless optimization to try to not do some more processing that will just bites us later on.

If we can "afford" to do twice the computation that was done until now, then sure, we can skip (2). This is because there is currently no straightforward way to pipe the traces from the tracing module directly to this new output and we have no other way than linearly filtering every sample to see which one has the trace_id attached. You can see this logic here:

k6/output/cloud/expv2/collect.go

Lines 193 to 248 in df922af

func (c *requestMetadatasCollector) CollectRequestMetadatas(sampleContainers []metrics.SampleContainer) {

if len(sampleContainers) < 1 {

return

}

// TODO(lukasz, other-proto-support): Support grpc/websocket trails.

httpTrails := c.filterTrailsWithTraces(sampleContainers)

if len(httpTrails) > 0 {

c.collectHTTPTrails(httpTrails)

}

}

func (c *requestMetadatasCollector) filterTrailsWithTraces(

sampleContainers []metrics.SampleContainer,

) []*httpext.Trail {

var filteredHTTPTrails []*httpext.Trail

for _, sampleContainer := range sampleContainers {

if trail, ok := sampleContainer.(*httpext.Trail); ok {

if _, found := trail.Metadata[tracing.MetadataTraceIDKeyName]; found {

filteredHTTPTrails = append(filteredHTTPTrails, trail)

}

}

}

return filteredHTTPTrails

}

func (c *requestMetadatasCollector) collectHTTPTrails(trails []*httpext.Trail) {

newBuffer := make(insights.RequestMetadatas, 0, len(trails))

for _, trail := range trails {

m := insights.RequestMetadata{

TraceID: trail.Metadata[tracing.MetadataTraceIDKeyName],

Start: trail.EndTime.Add(-trail.Duration),

End: trail.EndTime,

TestRunLabels: insights.TestRunLabels{

ID: c.testRunID,

Scenario: c.getStringTagFromTrail(trail, scenarioTag),

Group: c.getStringTagFromTrail(trail, groupTag),

},

ProtocolLabels: insights.ProtocolHTTPLabels{

URL: c.getStringTagFromTrail(trail, nameTag),

Method: c.getStringTagFromTrail(trail, methodTag),

StatusCode: c.getIntTagFromTrail(trail, statusTag),

},

}

newBuffer = append(newBuffer, m)

}

c.bufferMu.Lock()

defer c.bufferMu.Unlock()

c.buffer = append(c.buffer, newBuffer...)

}

If this is not a big issue with regard to compute, then sure, we don't have to bother.

CC @codebien

I am not really certain going through all the metrics and checking whether they have metadata will be all that much "compute" compared to everything else the output(s) need to do.

And we likely can do it in a single place, so that will be even less of an issue.

By a single place I mean that output v2 will aggregate and on the side check that a given metric sample has metadata - if it does - send it to the insights output.

Even without that I expect this to be an insignificant amount of work.

Also, again - we will only to this for the users that have this enabled, and I expect they will likely want to use that.

@Blinkuu maybe it's my misunderstanding, but my interpretation of (2) was that it would drop the requirement for the flag so if a user does k6 cloud script.js and it is importing the tracing module then we should enable this proxy of the metadata. If this is not required, then I agree with @mstoykov if the user has to explicit the flag there is no reason for doing more.

@mstoykov I'm not sure I'm following, do you mean we should enable the check of the metadata for all scripts that the cloud run? If not, then how do we decide when to enable it or not for the cloud runs without (3)?

Ideally, we would like to enable the request metadata output logic if all 3 conditions are met:

Given this sentence my only opinion is taht (2) is not really necessary for us to not have this enabled most of the times.

BUT:

It probably shouldn't be limitted based on 1 module given that users can generate their own trace_id metadata as well after this release

Even if it is enabled for a test that does not have metadata - I doubt there is any performance problem to be going through all the samples and checking if there is metadata on them.

I doubt there is any performance problem to be going through all the samples and checking if there is metadata on them.

We can quickly verify this assumption during the next cycle, so I see no problem in trying with it always enabled. 👍

@Blinkuu you can simplify the logic around here a lot then. 🎉

In the case, we experience a performance issue, we may consider adding a method on the SampleContainer like HasMetadata() or MetadataCount() and it should reduce the cost of this check from O(N) to O(1).

The idea behind K6_CLOUD_TRACES_ENABLED is to hide k6 x Tempo behind a feature flag until it's stable enough we can make it true by default. Hence I brought up (2). If you think this is unnecessary, let's skip this and revisit it later if we see big enough utilization spikes. I would like to keep the TODO for now tho.

Blinkuu · 2023-06-05T14:59:47Z

output/cloud/expv2/collect.go

+const (
+	scenarioTag = "scenario"
+	groupTag    = "group"
+	nameTag     = "name"


Is this name tag by default set to http_url if users don't specify a value for it? My current understanding is that this tag is used to deal with cardinality issues (source).

It is set by default if it is enabled - which it must be for the usage of the cloud output.

Other required tags for tis output include method, status, error, check and group

k6/output/cloud/output.go

Lines 134 to 154 in 3ab0085

func validateRequiredSystemTags(scriptTags *metrics.SystemTagSet) error {

missingRequiredTags := []string{}

requiredTags := metrics.SystemTagSet(metrics.TagName |

metrics.TagMethod |

metrics.TagStatus |

metrics.TagError |

metrics.TagCheck |

metrics.TagGroup)

for _, tag := range metrics.SystemTagValues() {

if requiredTags.Has(tag) && !scriptTags.Has(tag) {

missingRequiredTags = append(missingRequiredTags, tag.String())

}

}

if len(missingRequiredTags) > 0 {

return fmt.Errorf(

"the cloud output needs the following system tags enabled: %s",

strings.Join(missingRequiredTags, ", "),

)

}

return nil

}

What about the scenario tag?

mstoykov · 2023-06-06T12:11:31Z

cloudapi/insights/proto/gen.go

+// by the k6 Insights.
+package proto
+
+//go:generate sh gen.sh


I would prefer if you just embed the whole thing in here and not use find.

It will be nice if this actually has good chances of working on a windows machine, withouhg needing fixes.

Done in 369297e.

mstoykov · 2023-06-06T12:15:28Z

js/modules/k6/experimental/tracing/trace_id.go

+	// MetadataTraceIDKeyName is the key name of the traceID in the output metadata.
+	MetadataTraceIDKeyName = "trace_id"


nit: I am kind of against this being public and you using it.

Again this package is 99% likely to never get out of experimental and users will be able to add trace_id even without it.

It seems a lot more reasonable for the insights output to just document that certain metrics with the trace_id metadata will have their samples processed.

Having this exported to get this string seems completely unnecessary dependancy.

see https://go-proverbs.github.io/

Fixed in 8860d2f.

mstoykov · 2023-06-06T12:19:30Z

output/cloud/expv2/collect.go

+const (
+	scenarioTag = "scenario"
+	groupTag    = "group"
+	nameTag     = "name"


It is set by default if it is enabled - which it must be for the usage of the cloud output.

Other required tags for tis output include method, status, error, check and group

k6/output/cloud/output.go

Lines 134 to 154 in 3ab0085

func validateRequiredSystemTags(scriptTags *metrics.SystemTagSet) error {

missingRequiredTags := []string{}

requiredTags := metrics.SystemTagSet(metrics.TagName |

metrics.TagMethod |

metrics.TagStatus |

metrics.TagError |

metrics.TagCheck |

metrics.TagGroup)

for _, tag := range metrics.SystemTagValues() {

if requiredTags.Has(tag) && !scriptTags.Has(tag) {

missingRequiredTags = append(missingRequiredTags, tag.String())

}

}

if len(missingRequiredTags) > 0 {

return fmt.Errorf(

"the cloud output needs the following system tags enabled: %s",

strings.Join(missingRequiredTags, ", "),

)

}

return nil

}

mstoykov · 2023-06-06T12:22:14Z

cloudapi/config.go

+		MetricPushConcurrency: null.NewInt(1, false),
+
+		TracesEnabled:      null.NewBool(false, false),
+		TracesHost:         null.NewString("insights.k6.io", false),


All the other urls/hosts here include a schema, so at least to be consistent it is likely a good idea to keep it that way.

This is a gRPC service. Should I add the dns: prefix, then?

gRPC naming: https://github.com/grpc/grpc/blob/master/doc/naming.md

Hmmm ... of course grpc does not have grpc schema :(

Does this code support the other ones ipv4/ipv6 and or unix ?

Nope, it will just use plain DNS. I also updated the value to include a port. Now it is insights.k6.io:4443.

cloudapi/insights/client.go

output/cloud/expv2/collect.go

codebien · 2023-06-07T08:26:22Z

output/cloud/expv2/output.go

+	// TODO(lukasz): Check if the `tracing` module is imported.
+	//
+	// We want to check if the `tracing` module was imported. If
+	// it wasn't, we don't need to collect any data related to tracing
+	// as there won't be any.
+	//
+	// We currently don't have a way to check if the tracing module
+	// was imported by the user.


@Blinkuu maybe it's my misunderstanding, but my interpretation of (2) was that it would drop the requirement for the flag so if a user does k6 cloud script.js and it is importing the tracing module then we should enable this proxy of the metadata. If this is not required, then I agree with @mstoykov if the user has to explicit the flag there is no reason for doing more.

@mstoykov I'm not sure I'm following, do you mean we should enable the check of the metadata for all scripts that the cloud run? If not, then how do we decide when to enable it or not for the cloud runs without (3)?

cloudapi/insights/client.go

codebien · 2023-06-07T08:33:16Z

cloudapi/insights/proto/v1/common/common.proto

+
+option go_package = "go.k6.io/k6/cloudapi/insights/proto/v1/common";
+
+// Messages below are copied from https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/common/v1/common.proto


Is this copy legit? Are we respecting the license?

Not sure. I'm not a lawyer. Do we have somebody with more expertise in licensing that could chip in here?

I added a copyright notice in ad6ca70. AFAIK, this should be enough, but perhaps someone with more knowledge could double-check it.

cloudapi/insights/proto/v1/k6/labels.proto

output/cloud/expv2/output.go

vendor/modules.txt

codebien · 2023-06-13T16:03:25Z

cloudapi/insights/client.go

+	if c.conn == nil {
+		return ErrClientClosed
+	}
+	c.connMu.RUnlock()


Suggested change

if c.conn == nil {

return ErrClientClosed

}

c.connMu.RUnlock()

closed := c.conn == nil

c.connMu.RUnlock()

if closed {

return ErrClientClosed

}

Done in 3f7e461.

codebien · 2023-06-13T16:06:10Z

cloudapi/insights/client.go

+
+	req, err := newBatchCreateRequestMetadatasRequest(requestMetadatas)
+	if err != nil {
+		return fmt.Errorf("failed to create request from request metadatas: %w", err)


Suggested change

return fmt.Errorf("failed to create request from request metadatas: %w", err)

return fmt.Errorf("failed to create request from request metadata: %w", err)

Metadata doesn't have the plural, am I missing something? I see it is used all over the code so if we change then keep it consistent.

Yes, we are aware it doesn't have a plural form. However, together with @vortegatorres we decided to add the s suffix to match the conventions defined in Google API Design Guide. I'd like to keep this as "request metadatas".

I am not certain which part of the Google API Design Docs apply here ?

See https://google.aip.dev/233:

The RPC's name must begin with BatchCreate. The remainder of the RPC name should be the plural form of the resource being created.

We use singular for a single request and plural for batch requests (see here)

codebien · 2023-06-13T16:07:49Z

cloudapi/insights/client.go

+	if c.conn == nil {
+		return ErrClientClosed
+	}
+	c.connMu.RUnlock()


Is it the rest of the logic fault tolerant in the case the connection closes after it?

I added logic to close the client in d5ec11e.

In practice, we should never hit the ErrClientClosed if the client is used properly. We create the client and establish the connection on output startup, and then we close it in StopWithTestError(...). The ErrClientClosed is here to be the safeguard and an indication that the client is misused.

codebien · 2023-06-13T16:25:24Z

output/cloud/expv2/output.go

+// RequestMetadatasCollector is an interface for collecting request metadatas
+// and retrieving them so they can be flushed using a Flusher.
+type RequestMetadatasCollector interface {
+	CollectRequestMetadatas([]metrics.SampleContainer)
+	PopAll() insights.RequestMetadatas
+}
+
+// Flusher is an interface for flushing data to the cloud.
+type Flusher interface {
+	Flush(ctx context.Context) error
+}
+


Suggested change

// RequestMetadatasCollector is an interface for collecting request metadatas

// and retrieving them so they can be flushed using a Flusher.

type RequestMetadatasCollector interface {

CollectRequestMetadatas([]metrics.SampleContainer)

PopAll() insights.RequestMetadatas

}

// Flusher is an interface for flushing data to the cloud.

type Flusher interface {

Flush(ctx context.Context) error

}

// RequestMetadatasCollector is an interface for collecting request metadatas

// and retrieving them so they can be flushed using a Flusher.

type requestMetadatasCollector interface {

CollectRequestMetadatas([]metrics.SampleContainer)

PopAll() insights.RequestMetadatas

}

// Flusher is an interface for flushing data to the cloud.

type flusher interface {

flush(ctx context.Context) error

}

I'm going to open a PR for unexporting all the methods/interfaces/fields not required to be exported. So let's not add more. It sounds they are used only internally to the current package.

Fixed in dd1741f.

codebien · 2023-06-13T16:29:46Z

output/cloud/expv2/output.go

+	// Drain the SampleBuffer
+	o.collectSamples()
+
+	// Force the aggregation for flushing


A bad rebase? This operation is already called two lines below

Indeed. Fixed in 7c41939.

codebien · 2023-06-13T16:33:12Z

output/cloud/expv2/output.go

+	// We want to check if the `tracing` module was imported. If
+	// it wasn't, we don't need to collect any data related to tracing
+	// as there won't be any.
+	//
+	// We currently don't have a way to check if the tracing module
+	// was imported by the user.


Based on the previous discussion it is not required anymore, right?

Suggested change

// We want to check if the `tracing` module was imported. If

// it wasn't, we don't need to collect any data related to tracing

// as there won't be any.

//

// We currently don't have a way to check if the tracing module

// was imported by the user.

Removed in 19fa719.

codebien · 2023-06-19T12:34:35Z

Hey @Blinkuu, it seems there is a conflict, can you rebase it, please?

output/cloud/expv2/output.go

js/modules/k6/experimental/tracing/client.go

output/cloud/expv2/output.go

cloudapi/insights/proto/v1/trace/span.proto

output/cloud/expv2/output.go

codebien · 2023-06-19T15:52:18Z

output/cloud/expv2/flush_test.go

+func Test_tracesFlusher_Flush_ReturnsNoErrorWithWorkingInsightsClientAndNonCancelledContextAndData(t *testing.T) {
+	t.Parallel()
+
+	// Given


Out of curiosity, why do you think this practice is helpful?

If they work for you, I'm not against keeping them, but in my opinion, a unit test should try to achieve some level of auto-explanation and I think a test like the current already does.

The constructors are grouped, the logic method is only one, and the assert section is also grouped.

At this point, it's muscle memory. It helps me when I revisit some bigger unit tests.

codebien · 2023-06-19T15:57:48Z

Hey @mstoykov, I don't think we have an issue with the timeout here because the flush doesn't sound concurrent, but you may have another opinion regarding the timeout lifespan.

codebien

LGTM 🙇

EDIT: Of course, we need all tests green

cloudapi/config_test.go

mstoykov · 2023-06-21T15:31:10Z

cloudapi/insights/client.go

+
+	req, err := newBatchCreateRequestMetadatasRequest(requestMetadatas)
+	if err != nil {
+		return fmt.Errorf("failed to create request from request metadatas: %w", err)


I am not certain which part of the Google API Design Docs apply here ?

mstoykov · 2023-06-21T15:36:59Z

cloudapi/insights/domain.go

+	if rm.TestRunLabels.Group == "" {
+		return errEmptyTestRunGroup
+	}


A group being empty seems like pretty okay to me. Especially as that is what happens if you do not have a group and most users are okay with it.

This I guess is some backend restriction, but I will expect this will invalidate quite a few scripts that don't really have problems otherwise.

Scenario is always set ... but I also find it strange that you care about that 🤷

All the values must be present, this is how our backend works. The other part of the code should fill these gaps with "unknown" if not set.

The other part of the code should fill these gaps with "unknown" if not set.

But group can have (and does) an empty value for metrics emitted outside of a group.

So this validation (which is now removed) would've dropped all traces for request done outside of a group.

And if you do that on backside you will also do it.

The function that sets it to unknown only does it if it is missing not if the value is empty.

Which ... seems just as well to in practice be done on the backend. If you see an empty group - set it to unknwon there. Instead of setting it to unknown from here.

Specifically here unknown kind of ... "not allowed" value for group ... but even if it was I would not make a "magical" string be what I mean when a field is missing. The empty value seems just as well, especially if you don't want to differntiate between it and the thing not being set.

cloudapi/insights/mappers.go

output/cloud/expv2/collect.go

mstoykov · 2023-06-21T15:49:18Z

output/cloud/expv2/collect.go

+		return tag
+	}
+
+	return "unknown"


Won't it make more sense to leave it empty and just not handle it as an error ?

We rely on this being non-empty, at least for the time being.

mstoykov · 2023-06-21T16:16:05Z

output/cloud/expv2/output.go

+// requestMetadatasCollector is an interface for collecting request metadatas
+// and retrieving them, so they can be flushed using a flusher.
+type requestMetadatasCollector interface {
+	CollectRequestMetadatas([]metrics.SampleContainer)
+	PopAll() insights.RequestMetadatas
+}


nit: I don't particularly see why you need a interface here 🤷

I expect this is to facilitate testing only?

Yeah, this is currently unused. Do you want me to remove this interface?

If it is unused, then we should remove it. I don't see the point in having more abstraction, if not required.

As discussed in private, it's used in testing. Leaving as is.

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

It adds support for flushing the tracing metadata to the related Cloud's remote gRPC service. --------- Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Blinkuu self-assigned this May 29, 2023

Blinkuu force-pushed the implement_request_metadata_output branch 3 times, most recently from 0840b6e to 0a21265 Compare May 30, 2023 15:36

Blinkuu added the feature label May 30, 2023

Blinkuu force-pushed the implement_request_metadata_output branch from 84ebc9d to 121056d Compare May 31, 2023 11:02

Blinkuu changed the title ~~Implement RequestMetadata output~~ output/cloudv2: Implement RequestMetadata output May 31, 2023

Blinkuu commented May 31, 2023

View reviewed changes

Blinkuu requested a review from codebien May 31, 2023 12:58

Blinkuu marked this pull request as ready for review May 31, 2023 12:58

Blinkuu requested review from mstoykov and oleiade May 31, 2023 14:05

Blinkuu commented Jun 5, 2023

View reviewed changes

mstoykov reviewed Jun 6, 2023

View reviewed changes

codebien requested changes Jun 7, 2023

View reviewed changes

Blinkuu force-pushed the implement_request_metadata_output branch from 6db7e33 to 84fdae2 Compare June 13, 2023 08:36

Blinkuu requested review from codebien and mstoykov June 13, 2023 09:00

codebien requested changes Jun 13, 2023

View reviewed changes

codebien mentioned this pull request Jun 14, 2023

output/cloudv2: Retry and flushers pool #3104

Merged

Blinkuu requested a review from codebien June 19, 2023 11:20

codebien reviewed Jun 19, 2023

View reviewed changes

Blinkuu requested a review from codebien June 20, 2023 09:24

codebien previously approved these changes Jun 20, 2023

View reviewed changes

Blinkuu dismissed codebien’s stale review via 521e1e2 June 20, 2023 09:36

Blinkuu requested a review from codebien June 20, 2023 09:56

mstoykov reviewed Jun 21, 2023

View reviewed changes

Blinkuu and others added 23 commits June 23, 2023 10:57

Set HTTP URL label to name tag

5f7b124

Update traces host

7956a52

Remove gen.sh

1509fde

Make MetadataTraceIDKeyName private again

e6129b2

Update min TLS version to 1.3

3ffad13

Reimplement CollectRequestMetadatas()`

eb58b6e

Format flush_test.go

0f6448b

Add //nolint:lll directive

4e677f6

Add copyright notice to common.proto

817b18c

Make internal interfaces private

7afb9fa

Remove obsolete TODO

456da47

Minimize critical section

2db5ace

Add shutdown hooks

39f3da5

Fix after merge

b3e8f86

Remove argument name from the flusher interface

127f3a8

Update js/modules/k6/experimental/tracing/client.go

27cd2bc

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Store insightsClient instead of shutdownHooks

24602fb

Update output/cloud/expv2/output.go

1910235

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Update output/cloud/expv2/output.go

2b82672

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Fix flush_test.go

117858c

Bring back 5s timeout in config_test.go

029632e

Remove redundant validation logic

3fbac3e

Rollback changes in metric.pb.go

6ffe102

Blinkuu force-pushed the implement_request_metadata_output branch from a95ba44 to 6ffe102 Compare June 23, 2023 09:03

codebien approved these changes Jun 23, 2023

View reviewed changes

codebien merged commit 97d40e9 into master Jun 23, 2023

codebien deleted the implement_request_metadata_output branch June 23, 2023 11:17

This was referenced Jul 17, 2023

k6 Insights (1/2): Refactor request metadata output to a separate package #3201

Merged

k6 Insights (2/2): Integrate request metadata output to cloud output v1 #3202

Merged

	func (c *requestMetadatasCollector) CollectRequestMetadatas(sampleContainers []metrics.SampleContainer) {
	if len(sampleContainers) < 1 {
	return
	}

	// TODO(lukasz, other-proto-support): Support grpc/websocket trails.
	httpTrails := c.filterTrailsWithTraces(sampleContainers)
	if len(httpTrails) > 0 {
	c.collectHTTPTrails(httpTrails)
	}
	}

	func (c *requestMetadatasCollector) filterTrailsWithTraces(
	sampleContainers []metrics.SampleContainer,
	) []*httpext.Trail {
	var filteredHTTPTrails []*httpext.Trail

	for _, sampleContainer := range sampleContainers {
	if trail, ok := sampleContainer.(*httpext.Trail); ok {
	if _, found := trail.Metadata[tracing.MetadataTraceIDKeyName]; found {
	filteredHTTPTrails = append(filteredHTTPTrails, trail)
	}
	}
	}

	return filteredHTTPTrails
	}

	func (c requestMetadatasCollector) collectHTTPTrails(trails []httpext.Trail) {
	newBuffer := make(insights.RequestMetadatas, 0, len(trails))

	for _, trail := range trails {
	m := insights.RequestMetadata{
	TraceID: trail.Metadata[tracing.MetadataTraceIDKeyName],
	Start: trail.EndTime.Add(-trail.Duration),
	End: trail.EndTime,
	TestRunLabels: insights.TestRunLabels{
	ID: c.testRunID,
	Scenario: c.getStringTagFromTrail(trail, scenarioTag),
	Group: c.getStringTagFromTrail(trail, groupTag),
	},
	ProtocolLabels: insights.ProtocolHTTPLabels{
	URL: c.getStringTagFromTrail(trail, nameTag),
	Method: c.getStringTagFromTrail(trail, methodTag),
	StatusCode: c.getIntTagFromTrail(trail, statusTag),
	},
	}

	newBuffer = append(newBuffer, m)
	}

	c.bufferMu.Lock()
	defer c.bufferMu.Unlock()

	c.buffer = append(c.buffer, newBuffer...)
	}

	func validateRequiredSystemTags(scriptTags *metrics.SystemTagSet) error {
	missingRequiredTags := []string{}
	requiredTags := metrics.SystemTagSet(metrics.TagName \|
	metrics.TagMethod \|
	metrics.TagStatus \|
	metrics.TagError \|
	metrics.TagCheck \|
	metrics.TagGroup)
	for _, tag := range metrics.SystemTagValues() {
	if requiredTags.Has(tag) && !scriptTags.Has(tag) {
	missingRequiredTags = append(missingRequiredTags, tag.String())
	}
	}
	if len(missingRequiredTags) > 0 {
	return fmt.Errorf(
	"the cloud output needs the following system tags enabled: %s",
	strings.Join(missingRequiredTags, ", "),
	)
	}
	return nil
	}

		// MetadataTraceIDKeyName is the key name of the traceID in the output metadata.
		MetadataTraceIDKeyName = "trace_id"


		option go_package = "go.k6.io/k6/cloudapi/insights/proto/v1/common";

		// Messages below are copied from https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/common/v1/common.proto

	return fmt.Errorf("failed to create request from request metadatas: %w", err)
	return fmt.Errorf("failed to create request from request metadata: %w", err)

output/cloudv2: Implement RequestMetadata output #3100

output/cloudv2: Implement RequestMetadata output #3100

Conversation

Blinkuu commented May 29, 2023 • edited Loading

codecov-commenter commented May 31, 2023 • edited Loading

Codecov Report

Blinkuu May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blinkuu Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blinkuu Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blinkuu Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blinkuu Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien commented Jun 19, 2023

codebien left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blinkuu commented May 29, 2023 •

edited

Loading

codecov-commenter commented May 31, 2023 •

edited

Loading

Blinkuu May 31, 2023 •

edited

Loading

Blinkuu Jun 5, 2023 •

edited

Loading

codebien Jun 8, 2023 •

edited

Loading

Blinkuu Jun 12, 2023 •

edited

Loading

Blinkuu Jun 13, 2023 •

edited

Loading

Blinkuu Jun 19, 2023 •

edited

Loading

codebien left a comment •

edited

Loading

codebien Jun 22, 2023 •

edited

Loading