Broker tracing conventions #2983

ian-mi · 2020-04-14T22:44:47Z

Proposed Changes

Define conventions for ingress and trigger spans and for setting messaging span attributes.

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

ian-mi · 2020-04-14T23:18:19Z

cc @grantr @adamharwayne

matzew · 2020-04-20T08:06:10Z

pkg/mtbroker/filter/filter_handler.go

+	if span.IsRecordingEvents() {
+		span.AddAttributes(
+			tracing.MessagingSystemAttribute,
+			tracing.MessagingProtocolHTTP,
+			tracing.TriggerMessagingDestinationAttribute(triggerRef.NamespacedName),
+			tracing.MessagingMessageIDAttribute(event.ID()),
+		)
+	}


move to some util? It's like 4times in this PR 🤷‍♂️

I'm not sure a helper function would improve readability. All of these attributes besides the system attribute would need to be arguments to the helper function anyways and I prefer being explicit about what attributes are being added.

vaikas · 2020-05-04T17:00:41Z

@ian-mi would you mind getting this rebased? Also, thoughts on util for the span setting?

googlebot · 2020-05-08T21:47:37Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

ian-mi · 2020-05-08T21:48:43Z

Rebased! Needs consent from @mattmoor

vaikas · 2020-05-13T17:27:24Z

/lgtm
/approve

mattmoor · 2020-05-13T18:47:37Z

@googlebot I consent!

mattmoor · 2020-05-13T18:48:06Z

@googlebot I consent.

It is possible to directly test the attribute helpers since attribute fields are unexported.

googlebot · 2020-05-13T19:15:30Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

Co-authored-by: Matt Moore <mattmoor@vmware.com>

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

docs/spec/broker.md

Co-authored-by: Matt Moore <mattmoor@vmware.com>

knative-metrics-robot · 2020-05-13T19:24:27Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/broker/filter/filter_handler.go	78.4%	78.3%	-0.1
pkg/broker/ingress/ingress_handler.go	82.4%	84.6%	2.3
pkg/mtbroker/filter/filter_handler.go	78.4%	78.3%	-0.1
pkg/mtbroker/ingress/ingress_handler.go	83.3%	83.0%	-0.4
pkg/tracing/attributes.go	Do not exist	50.0%

ian-mi · 2020-05-13T19:39:13Z

Strange that that didn't seem to work... I've just dropped the commits and reapplied the suggestions to workaround the issue. I've also slightly improved test coverage.

vaikas · 2020-05-13T20:04:47Z

/lgtm
/approve

knative-prow-robot · 2020-05-13T20:04:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ian-mi, vaikas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 14, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2020

mattmoor reviewed Apr 14, 2020

View reviewed changes

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Apr 14, 2020

knative-prow-robot requested review from evankanderson and yt3liu April 14, 2020 22:45

mattmoor reviewed Apr 14, 2020

View reviewed changes

matzew reviewed Apr 20, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2020

ian-mi added 3 commits May 8, 2020 14:35

Add tracing conventions to broker spec

4563dc5

Add broker ingress spans with messaging span attributes

a9a9826

Add trigger span and messaging span attributes

6428605

ian-mi force-pushed the broker-tracing-conventions branch from 733f364 to c8a24c2 Compare May 8, 2020 21:47

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2020

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels May 8, 2020

knative-prow-robot assigned vaikas May 13, 2020

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 13, 2020

Add test coverage of attributes.go

67b17d7

It is possible to directly test the attribute helpers since attribute fields are unexported.

ian-mi force-pushed the broker-tracing-conventions branch from c8a24c2 to 67b17d7 Compare May 13, 2020 19:15

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels May 13, 2020

mattmoor reviewed May 13, 2020

View reviewed changes

Format docs/spec/broker.md

968cacb

Co-authored-by: Matt Moore <mattmoor@vmware.com>

mattmoor reviewed May 13, 2020

View reviewed changes

docs/spec/broker.md Outdated Show resolved Hide resolved

docs/spec/broker.md Outdated Show resolved Hide resolved

Format Markdown

900d098

Co-authored-by: Matt Moore <mattmoor@vmware.com>

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

knative-prow-robot merged commit 1689191 into knative:master May 13, 2020

ian-mi deleted the broker-tracing-conventions branch May 13, 2020 20:06

ian-mi mentioned this pull request May 13, 2020

GCP Broker Follows Eventing Conventions for Spans and Attributes. google/knative-gcp#1063

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broker tracing conventions #2983

Broker tracing conventions #2983

ian-mi commented Apr 14, 2020 •

edited

Loading

mattmoor left a comment

mattmoor left a comment

ian-mi commented Apr 14, 2020

matzew Apr 20, 2020

ian-mi May 8, 2020

vaikas commented May 4, 2020

googlebot commented May 8, 2020

ian-mi commented May 8, 2020

vaikas commented May 13, 2020

mattmoor commented May 13, 2020

mattmoor commented May 13, 2020

googlebot commented May 13, 2020

mattmoor left a comment

mattmoor left a comment

knative-metrics-robot commented May 13, 2020

ian-mi commented May 13, 2020

vaikas commented May 13, 2020

knative-prow-robot commented May 13, 2020

Broker tracing conventions #2983

Broker tracing conventions #2983

Conversation

ian-mi commented Apr 14, 2020 • edited Loading

Proposed Changes

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

ian-mi commented Apr 14, 2020

matzew Apr 20, 2020

Choose a reason for hiding this comment

ian-mi May 8, 2020

Choose a reason for hiding this comment

vaikas commented May 4, 2020

googlebot commented May 8, 2020

ian-mi commented May 8, 2020

vaikas commented May 13, 2020

mattmoor commented May 13, 2020

mattmoor commented May 13, 2020

googlebot commented May 13, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented May 13, 2020

ian-mi commented May 13, 2020

vaikas commented May 13, 2020

knative-prow-robot commented May 13, 2020

ian-mi commented Apr 14, 2020 •

edited

Loading