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

rewrite focusing on telemetry #21

Merged
merged 2 commits into from
Aug 18, 2020
Merged

rewrite focusing on telemetry #21

merged 2 commits into from
Aug 18, 2020

Conversation

the-mikedavis
Copy link
Contributor

phoenix emits a telemetry event when it finishes a run with a conn and on that telemetry event is the conn and duration (exactly what we need)

so this PR changes the Hummingbird plug to solely determine trace IDs and put them into conn.private.hummingbird along with other metadata

when the telemetry event comes along, it'll trigger a telemetry handler Hummingbird.Telemetry (put that in your application sup tree) which will handle the sending of events to honeycomb and grabbing of metadata from the conn

a few other things:

  • a helper function for all b3 headers (for use in in-app requests)
  • respect the b3 sampled header (determines whether or not you should emit an event to honeycomb)
  • set aside a field called events: [] in conn.private.hummingbird
    • this should work nicely in the future when we want to add more events to a batch, like database requests or other functions

@the-mikedavis the-mikedavis requested a review from a team August 18, 2020 18:25
@the-mikedavis the-mikedavis self-assigned this Aug 18, 2020
Copy link
Member

@tonyvanriet tonyvanriet left a comment

Choose a reason for hiding this comment

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

that's a lot...
i approve

@the-mikedavis the-mikedavis merged commit f595ef3 into master Aug 18, 2020
@the-mikedavis the-mikedavis deleted the rewrite-telemetry branch August 18, 2020 18:26
|> put_req_header("x-b3-spanid", UUID.uuid4())
|> put_req_header("x-b3-traceid", UUID.uuid4())
|> put_req_header("x-b3-sampled", "0")
Copy link
Contributor

Choose a reason for hiding this comment

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

@the-mikedavis Only real concern here is that Defer sampling is done by absence of this header. 0 is deny and that seems ham-handed.

It makese sense to me to have each app have ownership of how it wants to set sampling or nah, especially since it should be able to trace according to its own rules. If it wants to make another trace, and link the two traces, that's its perogative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this header is just for the test though. there's another line somewhere setting the default when deferred to be true or "1" but I agree that should be configurable on a per app basis. something like config :hummingbird, sample_when_deferred: true|false

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 my bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#22 made a little issue for it here

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.

3 participants