Add k8s trace attribute to PingSource #5928

lionelvillard · 2021-11-22T21:48:33Z

Partial fix for #5888

Since OpenTelemetry does not support custom resources (yet), I chose what I think are reasonable span attribute names.

Proposed Changes

🎁 Traces generated by PingSource includes some k8s attributes (k8s.namespace, k8s.name, k8s.resource).

Pre-review Checklist

At least 80% unit test coverage
(NA) E2E tests for any new behavior
(NA) Docs PR for any user-facing impact
(NA) Spec PR for any new API feature
(NA) Conformance test for any change to the spec

Release Note

 :gift: Traces generated by PingSource includes some k8s attributes  (`k8s.namespace`, `k8s.name`, `k8s.resource`).

Docs

codecov · 2021-11-22T21:56:12Z

Codecov Report

Merging #5928 (813a398) into main (d5201e7) will increase coverage by 0.27%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #5928      +/-   ##
==========================================
+ Coverage   82.02%   82.30%   +0.27%     
==========================================
  Files         220      225       +5     
  Lines        7527     7628     +101     
==========================================
+ Hits         6174     6278     +104     
+ Misses        918      910       -8     
- Partials      435      440       +5

Impacted Files	Coverage Δ
pkg/observability/client/observable.go	`40.00% <40.00%> (ø)`
pkg/adapter/v2/cloudevents.go	`80.80% <50.00%> (-2.71%)`	⬇️
pkg/observability/client/observability_service.go	`73.91% <73.91%> (ø)`
pkg/adapter/mtping/runner.go	`84.21% <100.00%> (+0.87%)`	⬆️
pkg/observability/attributes.go	`100.00% <100.00%> (ø)`
pkg/observability/context.go	`100.00% <100.00%> (ø)`
pkg/reconciler/broker/config.go	`76.47% <0.00%> (-10.20%)`	⬇️
pkg/kncloudevents/message_sender.go	`86.27% <0.00%> (-3.39%)`	⬇️
pkg/adapter/apiserver/delegate.go	`88.23% <0.00%> (ø)`
pkg/apis/eventing/v1/trigger_types.go	`100.00% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5201e7...813a398. Read the comment docs.

matzew · 2021-11-23T07:03:24Z

what about other source?

odacremolbap

Added minor comments.
I think result tag would be very welcomed.

pkg/tracing/client/observability_service.go

odacremolbap · 2021-11-23T09:31:24Z

pkg/tracing/client/observability_service.go

+	return ctx, func(errOrResult error) {
+		span.End()
+	}


The CE SDK implementation seems to bring a valuable tag to the span:

A result reporter is created per span:

https://github.com/cloudevents/sdk-go/blob/1170e89edb9b504a806f2c6a26563c3c26b68276/observability/opencensus/v2/client/observability_service.go#L44

And deferred to check the outcome, depending on it a tag is added:

https://github.com/cloudevents/sdk-go/blob/1170e89edb9b504a806f2c6a26563c3c26b68276/observability/opencensus/v2/client/observability_service.go#L50-L57

https://github.com/cloudevents/sdk-go/blob/1170e89edb9b504a806f2c6a26563c3c26b68276/observability/opencensus/v2/client/reporter.go#L77-L84

Do you think something like that could be added to this PR's implementation?

Thanks for bringing this up. The reason I removed the reporter is because it's about metrics, not traces, and I have on my TODO list to check if this is not redundant with stats reporter... and it's not. We need to have it for not breaking the existing behavior (though we don't document it)

pkg/tracing/client/observability_service_test.go

pierDipi

Since OpenTelemetry does not support custom resources (yet), I chose what I think are reasonable span attribute names.

Should we propose one before we move too far down our custom names?

pkg/tracing/attributes.go

lionelvillard · 2021-11-23T14:23:54Z

what about other source?

For core sources it is tracked in this issue: #5888. Does not apply to SinkBinding and ContainerSource as the adapter is external.

I can do APIServerSource in this PR.

lionelvillard · 2021-11-23T14:28:40Z

Added minor comments.
I think result tag would be very welcomed.

Definitively, though maybe not in the PR as I'm not sure what should be the value. It could be as simple as ACK/NACK, but we may want something more/different. I feel this deserve its own issue, WDYT @odacremolbap ?

odacremolbap · 2021-11-23T14:59:03Z

Added minor comments.
I think result tag would be very welcomed.

Definitively, though maybe not in the PR as I'm not sure what should be the value. It could be as simple as ACK/NACK, but we may want something more/different. I feel this deserve its own issue, WDYT @odacremolbap ?

sure, that can go to a different issue.

knative-metrics-robot · 2022-01-06T20:11:39Z

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/adapter/mtping/runner.go	91.5%	91.8%	0.3
pkg/adapter/v2/cloudevents.go	88.0%	86.7%	-1.3
pkg/observability/attributes.go	Do not exist	100.0%
pkg/observability/client/observability_service.go	Do not exist	76.2%
pkg/observability/client/observable.go	Do not exist	50.0%
pkg/observability/context.go	Do not exist	100.0%

lionelvillard · 2022-01-06T20:17:40Z

@odacremolbap I moved everything to the observability package and added the result tag to preserve the existing behavior. It's a bit messy right now but I would rather not invest too much time on this until we migrate to OpenTelemetry. At that point we can fold stats_reporter into the OpenTelemetry equivalent of the observer service introduced in this PR.

@pierDipi The tracing semantic conventions for k8s are experimental anyway so I wouldn't wait. I'll update the doc to say these are experimental tracing attributes. Is that ok?

pierDipi

/lgtm

knative-prow-robot · 2022-01-07T07:07:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, pierDipi

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~~ [lionelvillard,pierDipi]

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

pierDipi · 2022-01-07T07:07:25Z

/hold @odacremolbap review

pierDipi · 2022-01-07T07:07:53Z

(too late, we can iterate in a follow up)

odacremolbap · 2022-01-07T08:50:24Z

pkg/adapter/v2/cloudevents.go

 )

-var newClientHTTPObserved = cloudeventsobsclient.NewClientHTTP
+var newClientHTTPObserved = NewClientHTTPObserved


nit: maybe newClientHTTPObserved is no longer needed and can be replaced with the public NewClientHTTPObserved

odacremolbap · 2022-01-07T08:52:04Z

No worries @pierDipi , this also lgtm.
I have only one minor comment that can be addressed when we migrate to OpenTelemetry.

👍

Add k8s trace attribute to PingSource

340ce48

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 22, 2021

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 22, 2021

odacremolbap reviewed Nov 23, 2021

View reviewed changes

pierDipi reviewed Nov 23, 2021

View reviewed changes

pkg/tracing/attributes.go Outdated Show resolved Hide resolved

lionelvillard mentioned this pull request Dec 2, 2021

Audit and implement traces for RabbitMQ Eventing pieces knative-extensions/eventing-rabbitmq#414

Closed

fixes after first review

813a398

pierDipi approved these changes Jan 7, 2022

View reviewed changes

knative-prow-robot assigned pierDipi Jan 7, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2022

knative-prow-robot merged commit 7bf054f into knative:main Jan 7, 2022

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2022

odacremolbap reviewed Jan 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add k8s trace attribute to PingSource #5928

Add k8s trace attribute to PingSource #5928

lionelvillard commented Nov 22, 2021

codecov bot commented Nov 22, 2021 •

edited

Loading

matzew commented Nov 23, 2021

odacremolbap left a comment

odacremolbap Nov 23, 2021

lionelvillard Nov 23, 2021

pierDipi left a comment

lionelvillard commented Nov 23, 2021

lionelvillard commented Nov 23, 2021

odacremolbap commented Nov 23, 2021

knative-metrics-robot commented Jan 6, 2022

lionelvillard commented Jan 6, 2022

pierDipi left a comment

knative-prow-robot commented Jan 7, 2022

pierDipi commented Jan 7, 2022

pierDipi commented Jan 7, 2022

odacremolbap Jan 7, 2022 •

edited

Loading

odacremolbap commented Jan 7, 2022

Add k8s trace attribute to PingSource #5928

Add k8s trace attribute to PingSource #5928

Conversation

lionelvillard commented Nov 22, 2021

Proposed Changes

Pre-review Checklist

codecov bot commented Nov 22, 2021 • edited Loading

Codecov Report

matzew commented Nov 23, 2021

odacremolbap left a comment

Choose a reason for hiding this comment

odacremolbap Nov 23, 2021

Choose a reason for hiding this comment

lionelvillard Nov 23, 2021

Choose a reason for hiding this comment

pierDipi left a comment

Choose a reason for hiding this comment

lionelvillard commented Nov 23, 2021

lionelvillard commented Nov 23, 2021

odacremolbap commented Nov 23, 2021

knative-metrics-robot commented Jan 6, 2022

lionelvillard commented Jan 6, 2022

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 7, 2022

pierDipi commented Jan 7, 2022

pierDipi commented Jan 7, 2022

odacremolbap Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

odacremolbap commented Jan 7, 2022

codecov bot commented Nov 22, 2021 •

edited

Loading

odacremolbap Jan 7, 2022 •

edited

Loading