Add links and events to tracing payload #21

ie-pham · 2023-12-28T19:16:33Z

test outputs against tempo 3163

added "event attribute" in commit to verify changes took place during testing

trace json from querying trace by id

"events": [
  {
      "timeUnixNano": "1703790751493000000",
      "name": "k6.z82CnPfdkFVs",
      "attributes": [
          {
              "key": "event attributek6.qrbAb",
              "value": {
                  "stringValue": "k6.tZLSTRQUJzo8"
              }
          }
      ]
  }
],

"links": [
  {
      "traceId": "K43tbrFZhQRSPbpFbe6irw==",
      "spanId": "aZQKClAJ5tc=",
      "attributes": [
          {
              "key": "k6.lgCDAL7OExDN",
              "value": {
                  "stringValue": "k6.2W4d5oKDI7nq"
              }
          }
      ]
  }
],

CLAassistant · 2023-12-28T19:16:40Z

All committers have signed the CLA.

stoewer · 2023-12-28T23:31:17Z

The PR looks good. However, it would be great to have some more capabilities regarding links

By default the xk6-client-tracing docker image runs the script ./examples/template/template.js which uses the TemplatedGenerator and not the ParameterizedGenerator. Therefore it would be nice to add the possibility to send links to the TemplatedGenerator too
It would be useful if there was a parameter to tune the link generation in the ParameterizedGenerator

zalegrala

This looks good to me. Nice addition.

stoewer

Nice PR. I suggested a couple of changes to make the new functionality more consistent with the existing behavior

stoewer · 2024-01-15T06:33:29Z

examples/template/template.js

+            {service: "shop-backend", attributes: {"http.status_code": 403}},
+            {service: "shop-backend", name: "authenticate", attributes: {"http.request.header.accept": ["application/json"]}},
+            {service: "auth-service", name: "authenticate", attributes: {"http.status_code": 403}},
+            {service: "test-random", name: "events", randomEvents: {exceptionCount: 1, count: 2, randomAttributes: {count: 5, cardinality: 2}}},


In this template the span and service names etc. mimic traces from a somewhat plausible e-commerce service landscape. It would be good to keep this and choose less generic service (test-random) and span names (links, events) here

stoewer · 2024-01-15T07:18:06Z

pkg/tracegen/templated.go

+	RandomAttributes *AttributeParams `js:"randomAttributes"`
+}
+
+type LinkDefaults struct {


For randomly generated attributes the struct is called AttributeParams and it is used in SpanTemplate and SpanDefaults. Do you think we can do the same for links i.e. merge LinkDefaults and RandomLinks to LinkParams? If you use Rate instead of Count it should be possible to merge LinkDefaults and RandomLinks into one struct without losing functionality.

The main advantage beside consistency is that users don't have to remember two similar sets of parameters and where to use which one.

stoewer · 2024-01-15T07:22:14Z

pkg/tracegen/templated.go

+type LinkDefaults struct {
+	// LinkToPreviousSpanIndex true will set TraceID and SpanID the same as the previous span
+	// Unless it is the first span then a random TraceID and a random SpanID will be used
+	LinkToPreviousSpanIndex bool `js:"linkToPreviousSpanIndex"`


I'm wondering if we could make true the default for all links and remove this option. The reason is that this option is very specific and does not leave much room for future changes in the linking behavior without making this option obsolete. Maybe we want to link across traces in the future or link to random siblings in the same trace ...

agreed. will just make it link to the previous span as the default until we can support linking to specific trace/span in the future

stoewer · 2024-01-15T07:23:35Z

pkg/tracegen/templated.go

+	RandomAttributes *AttributeParams `js:"randomAttributes"`
+}
+
+type EventDefaults struct {


Same comment as for links about merging EventDefaults and RandomEvents to EventParams

stoewer · 2024-01-15T07:32:50Z

pkg/tracegen/templated.go

+			RandomAttributes:         defaults.EventDefaults.RandomAttributes,
+			Count:                    int(eventDefaultsRate),
+		}
+	} else if idx%int(1/eventDefaultsRate) == 0 {


I think it would be better to use a random factor instead of the index to decide whether to add an event. The condition could look like this: random.Float() < eventDefaultsRate

stoewer · 2024-01-15T07:33:29Z

pkg/tracegen/templated.go

+			RandomAttributes:        defaults.LinkDefaults.RandomAttributes,
+			Count:                   int(linkDefaultsRate),
+		}
+	} else if idx%int(1/linkDefaultsRate) == 0 {


Similar comment as for events: consider using a random factor instead of the index

stoewer · 2024-01-15T07:35:13Z

pkg/tracegen/templated.go

+	// Random events generated for each span
+	EventDefaults EventDefaults `js:"eventDefaults"`
+	// Random links generated for each span
+	LinkDefaults LinkDefaults `js:"linkDefaults"`


Rename to RandomEvents and RandomLinks in order to be more consistent with the existing RandomAttributes

…ic linkage

stoewer · 2024-01-16T00:53:26Z

examples/template/template.js

-    randomAttributes: {count: 2, cardinality: 5}
+    randomAttributes: {count: 2, cardinality: 5},
+    eventDefaults: {generateExceptionOnError: true, count: 1.0, randomAttributes: {count: 2, cardinality: 3}},
+    linkDefaults: {linkToPreviousSpanIndex: true, count: 1.0, randomAttributes: {count: 2, cardinality: 3}},


It looks like this was not adjusted to the latest changes

stoewer · 2024-01-16T00:55:04Z

pkg/tracegen/templated.go

+	EventDefaults EventParams `js:"eventDefaults"`
+	// Random links generated for each span
+	LinkDefaults LinkParams `js:"linkDefaults"`


Please rename to RandomEvents and RandomLinks in order to be consistent with RandomAttributes and field names in SpanTemplate

stoewer · 2024-01-16T04:47:31Z

examples/template/template.js

+            {service: "shop-backend", name: "authenticate", attributes: {"http.request.header.accept": ["application/json"]}},
+            {service: "auth-service", name: "authenticate", attributes: {"http.status_code": 403}},
+            {service: "cart-service", name: "checkout", randomEvents: {exceptionCount: 1, count: 2, randomAttributes: {count: 5, cardinality: 2}}},
+            {service: "billing-service", name: "payment", randomLinks: {linkToPreviousSpanIndex: true, count: 2, randomAttributes: {count: 3, cardinality: 2}}}


This was also not updated to the latest changes

stoewer · 2024-01-16T05:05:45Z

examples/template/template.js

+            {service: "shop-backend", attributes: {"http.status_code": 403}},
+            {service: "shop-backend", name: "authenticate", attributes: {"http.request.header.accept": ["application/json"]}},
+            {service: "auth-service", name: "authenticate", attributes: {"http.status_code": 403}},
+            {service: "cart-service", name: "checkout", randomEvents: {exceptionCount: 1, count: 2, randomAttributes: {count: 5, cardinality: 2}}},


When I test this together with the changes in PR grafana/tempo#3163 the events look a bit odd.

The timestamp is a negative value. In the trace data returned by tempo the timestamp is zero.

Instead of an event name with the value "exception" there is a field message with the value {"Type": "STRING", "Value": "exception"}

TBH I'm not sure about the origin of those problems. Maybe there is also a front-end issue?

oh weird, I am using grafana/grafana:main and I do see message instead of name but it just shows "exception" not a struct

I did notice that if timestamp is not provided, the front-end will display a negative number

tempo:latest

tempo:pr3163

ie-pham · 2024-01-16T19:20:04Z

I pushed my latest changes:

rename EventDefaults => RandomEvents, LinkDefaults => RandomLinks
add timestamp to exceptions
fix the example template

add links

017e400

ie-pham mentioned this pull request Dec 28, 2023

Send tracing payload with events, arrays (header), and links #17

Closed

remove test artifact

61a9688

ie-pham self-assigned this Jan 4, 2024

ie-pham changed the title ~~add links~~ Add links and events to tracing payload Jan 4, 2024

add templated events and links

58a4aa5

zalegrala approved these changes Jan 12, 2024

View reviewed changes

stoewer requested changes Jan 15, 2024

View reviewed changes

ie-pham added 2 commits January 15, 2024 16:46

use random factor for rates & merge defaults+random

39a488a

making linking to previous span a default until we can support specif…

cd0d018

…ic linkage

ie-pham requested a review from stoewer January 15, 2024 23:03

lint

723b5d8

stoewer requested changes Jan 16, 2024

View reviewed changes

fix template example, rename defaults

dcc463e

oops forgot to undo testing

c24f7d9

stoewer approved these changes Jan 17, 2024

View reviewed changes

ie-pham merged commit e340ce8 into grafana:main Jan 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add links and events to tracing payload #21

Add links and events to tracing payload #21

ie-pham commented Dec 28, 2023 •

edited

Loading

CLAassistant commented Dec 28, 2023 •

edited

Loading

stoewer commented Dec 28, 2023

zalegrala left a comment

stoewer left a comment

stoewer Jan 15, 2024

stoewer Jan 15, 2024

stoewer Jan 15, 2024

ie-pham Jan 15, 2024

stoewer Jan 15, 2024

stoewer Jan 15, 2024

ie-pham Jan 15, 2024

stoewer Jan 15, 2024

stoewer Jan 15, 2024

stoewer Jan 16, 2024

stoewer Jan 16, 2024

stoewer Jan 16, 2024

stoewer Jan 16, 2024 •

edited

Loading

ie-pham Jan 16, 2024 •

edited

Loading

ie-pham commented Jan 16, 2024

Add links and events to tracing payload #21

Add links and events to tracing payload #21

Conversation

ie-pham commented Dec 28, 2023 • edited Loading

CLAassistant commented Dec 28, 2023 • edited Loading

stoewer commented Dec 28, 2023

zalegrala left a comment

Choose a reason for hiding this comment

stoewer left a 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

Choose a reason for hiding this comment

stoewer Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

ie-pham Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

ie-pham commented Jan 16, 2024

ie-pham commented Dec 28, 2023 •

edited

Loading

CLAassistant commented Dec 28, 2023 •

edited

Loading

stoewer Jan 16, 2024 •

edited

Loading

ie-pham Jan 16, 2024 •

edited

Loading