Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept both B3 and TraceContext style tracing #3388

Merged
merged 7 commits into from
Jun 24, 2020

Conversation

Harwayne
Copy link
Contributor

Proposed Changes

  • Accept both B3 and TraceContext style tracing
  • Only TraceContext style tracing is egressed.
    • This was done for backwards compatibility with the existing system, which sends via TraceContext style. We could output both formats, but it was determined that the additional overhead of ~100 bytes per message was too high for the potential benefit. See here for more discussion.

Release Note

Eventing components now recognize both B3 and TraceContext style tracing.

…ontext style.

Egressing only TraceContext style is done for backwards compatibility with the existing system, which sends TraceContext style. We could output both styles, but it was determined that the additional overhead of ~100 bytes on every outgoing message was not worth it.
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 23, 2020
@knative-prow-robot 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. area/test-and-release Test infrastructure, tests or release labels Jun 23, 2020
@matzew
Copy link
Member

matzew commented Jun 24, 2020

/assign @slinkydeveloper

Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits, but lgtm

@@ -188,7 +189,9 @@ func vegetaAttackerTransport() *http.Transport {
func newCloudEventsClient(sinkUrl string) (cloudevents.Client, error) {
t, err := cloudevents.NewHTTP(
cloudevents.WithTarget(sinkUrl),
)
cloudevents.WithRoundTripper(&ochttp.Transport{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need tracing in performance images, it's not even configured

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

t, err := cloudevents.NewHTTP(cloudevents.WithPort(8080))
t, err := cloudevents.NewHTTP(
cloudevents.WithPort(8080),
cloudevents.WithRoundTripper(&ochttp.Transport{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This image uses only the receiver, so you don't need WithRoundTripper, because it's used only for the underlying HTTP client, for the server side you need to use WithMiddleware (you could easily reuse kncloudevents.CreateHandler for this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

t, err := cloudevents.NewHTTP(cloudevents.WithPort(8080))
t, err := cloudevents.NewHTTP(
cloudevents.WithPort(8080),
cloudevents.WithRoundTripper(&ochttp.Transport{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2020
@knative-metrics-robot
Copy link

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/v2/cloudevents.go 57.1% 58.0% 0.9

@slinkydeveloper
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne, slinkydeveloper

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 [Harwayne,slinkydeveloper]

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

@knative-prow-robot knative-prow-robot merged commit eb8ec30 into knative:master Jun 24, 2020
@Harwayne Harwayne deleted the tracecontext-egress branch June 24, 2020 19:12
lberk pushed a commit to lberk/eventing that referenced this pull request Jun 26, 2020
* hack/update-deps.sh

* Accept both TraceContext and B3 style trace formats. Send only TraceContext style.

Egressing only TraceContext style is done for backwards compatibility with the existing system, which sends TraceContext style. We could output both styles, but it was determined that the additional overhead of ~100 bytes on every outgoing message was not worth it.

* Update CE clients.

* hack/update-deps.sh

* PR comments.

* Undo changes to http_load_generator.go.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/performance area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants