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

281 basic distributed tracing #295

Merged
merged 59 commits into from
Jul 28, 2020
Merged

281 basic distributed tracing #295

merged 59 commits into from
Jul 28, 2020

Conversation

Knappek
Copy link
Contributor

@Knappek Knappek commented Mar 21, 2020

Description

Added

  • respect tracing headers for incoming HTTP requests
  • add tracing info to SSE welcome_event

Closes #281

follow-up: #309

What to look out for

Dear reviewer, I want you to

  • gain knowledge - I think it's important for you to know about the change.
  • check that the code works on your machine.
  • suggest implementation-code design/structure/readability improvements.
  • suggest test-code design/structure/readability improvements.
  • suggest documentation improvements.
  • suggest language/writing improvements.
  • help to find bugs (described below!).

Process

The goal is to improve not only the code in this PR but also our skills! The "rules":

  • The review is considered "done" as soon as all reviewers have added their review, and all their comments have been addressed.
  • For knowledge-sharing reviews, each reviewer should "approve" the PR after studying its content.
  • After the approval, the merge is concluded by the developer.

ToDo

  • Create child span and forward it
    • On API Gateway: take tracing and forward with new spanID to backend
      • Http: example
      • Kafka (fire and forget): example
      • Kinesis (fire and forget): example - this is not yet working with Jaeger/OpenZipkin; fails in opencensus plug
    • other direction: from backend to Frontend (here we only want to send traceparent to the frontend and not tracestate)
      • Kafka -> RIG -> Frontend: example
      • Kinesis -> RIG -> Frontend
      • Kafka -> RIG -> HTTP: ensure trace context is in http header
  • Display at Jaeger

Have fun!

@Azer0s
Copy link
Collaborator

Azer0s commented Mar 24, 2020

Just saw that you just added tracing for SSE. With the VConnection (which is hopefully being merged sometime soon), you can add tracing for all connection types (except for longpolling...but I'll try to get that working with the VConnection soon) in one fell swoop.

Just wanted to clarify so we don't end up with pesky merge conflicts 😄

@kevinbader kevinbader added this to the 3.0.0 milestone May 4, 2020
@Knappek Knappek marked this pull request as ready for review July 16, 2020 11:45
@Knappek Knappek removed their assignment Jul 16, 2020
Copy link
Contributor

@kevinbader kevinbader left a comment

Choose a reason for hiding this comment

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

changelog is missing

kevinbader
kevinbader previously approved these changes Jul 27, 2020
@Knappek Knappek assigned Knappek and unassigned kevinbader Jul 27, 2020
@Knappek Knappek force-pushed the 281-basic-distributed-tracing branch 2 times, most recently from 218c3ae to 233ff3e Compare July 28, 2020 07:20
@Knappek Knappek merged commit 925d764 into master Jul 28, 2020
@Knappek Knappek deleted the 281-basic-distributed-tracing branch October 2, 2020 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for OpenTracing
3 participants