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

basic tracing support draft #350

Merged
merged 43 commits into from
Feb 6, 2020
Merged

Conversation

guygrigsby
Copy link
Contributor

@guygrigsby guygrigsby commented Jan 18, 2020

Here we have a draft PR for Open Telemetry support. It includes span propagation from the client through the proxy and adds the events below. While initially I intended to implement all of them, time restraints have limited what I can do. That being said, I wanted to get this out there so Trickster has minimal tracing support for 1.0 and we can build it up as we go.

The requested support for initial tracing is as follows: (pulled from #36 )

  • upstream request time
  • fast forward request time
  • upstream revalidation request time
  • cache query time (also recording the query result in the span)
  • cache write time (recording # bytes written into the span)

As I was testing the tracing, I noticed that some traces are happening twice. The more I dug into it, the more I had a suspicion that it wasn't the tracing, but the tracing was exposing that we are making requests twice. This may be expected behavior that I don't understand, but I thought it was worth pointing out. Resolved.

@jranson
Copy link
Member

jranson commented Jan 18, 2020

What a Guy! Thank you, sir. Do you happen to know which spans are doubling up? In the delta proxy cache, this would be possible (or probable?), depending upon the spans and how they are observed. For example, if Trickster needs to make parallel requests to the upstream origin, for fast forward, or for reconstituting disparate ranges in the cache to fulfill the client request, that may register the spans instrumented during the upstream request process multiple times.

@guygrigsby guygrigsby force-pushed the open_telemetry branch 2 times, most recently from 3abbea0 to 3438b6b Compare January 18, 2020 05:48
@guygrigsby

This comment has been minimized.

@guygrigsby
Copy link
Contributor Author

guygrigsby commented Jan 18, 2020

I was sending multiple queries in a single request. Disregard duplicate comment. It will be hidden.

@guygrigsby guygrigsby force-pushed the open_telemetry branch 2 times, most recently from 90e5024 to dc88891 Compare January 20, 2020 16:20
@jranson
Copy link
Member

jranson commented Jan 22, 2020

One general change I would prefer is that we scope a TracingConfig to an Origin, rather than to the Trickster process, as there is a possibility different Origins will need different TracingConfigs (or none at all). I think we can easily change this out for the same map[string]*__Config approach we use for mapping caches and negative caches to an origin. There may be implications with the context to address as a result, but should be minimal effort.

@claassistantio
Copy link

claassistantio commented Jan 30, 2020

CLA assistant check
All committers have signed the CLA.

@jranson jranson merged commit 2efc9c0 into trickstercache:master Feb 6, 2020
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.

4 participants