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

Adds CollectedSpanHandler #2807

Open
wants to merge 3 commits into
base: master
from

Conversation

@adriancole
Copy link
Contributor

commented Sep 18, 2019

This starts #2802 by implementing low-hanging fruit: single span handler.

This one is easy because it fits directly into the existing collector infrastructure. To show how nicely it does, this backports CollectorSampler as a CollectedSpanHandler

@adriancole adriancole requested a review from jeqo Sep 18, 2019
@adriancole adriancole force-pushed the collected-span-handler branch from 185908a to 39bab62 Sep 19, 2019
* @param span model as {@link SpanBytesDecoder decoded} or handled upstream.
* @return the input if unmodified, a derived value, or null to drop this span.
*/
@Nullable Span handle(Span span);

This comment has been minimized.

Copy link
@adriancole

adriancole Sep 26, 2019

Author Contributor

here is the important type

public static Collector collector(ZipkinHttpCollector httpCollector) {
return httpCollector.collector;
}
}

This comment has been minimized.

Copy link
@devinsba

devinsba Sep 27, 2019

Member

Interesting pattern. I think I like this more than marking something "VisibleForTesting"

Copy link
Member

left a comment

I could be missing something but should we also add how these handlers will be mounted? (e.g. via classpath, similar to storages)

adriancole added a commit to openzipkin/brave that referenced this pull request Oct 2, 2019
It is a worse bug to allow duplicate handlers than it is to allow
duplicate configuration. Duplicate handlers translate directly into
runtime overhead, particularly as there is special casing for one
handler.

Throwing an exception was suggested here #961, but that is not a good
choice as it would almost certainly lead to someone having to do an
emergency release to undo it. This changes to logging on dupe, so that
we don't propagate this problem into future work such as collector
handlers.

See openzipkin/zipkin#2807 (comment)
adriancole added a commit to openzipkin/brave that referenced this pull request Oct 2, 2019
It is a worse bug to allow duplicate handlers than it is to allow
duplicate configuration. Duplicate handlers translate directly into
runtime overhead, particularly as there is special casing for one
handler.

Throwing an exception was suggested here #961, but that is not a good
choice as it would almost certainly lead to someone having to do an
emergency release to undo it. This changes to logging on dupe, so that
we don't propagate this problem into future work such as collector
handlers.

See openzipkin/zipkin#2807 (comment)
@adriancole

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@jeqo yeah it would be via adding to the classpath using "normal spring" but it is true most probably don't know how to do that. I have a local project I'll resume tonight or tomorrow.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

PS occurred to me that we could use a collected span handler to clean up data which results in needless index load, such as the '0' and '-' tags envoy adds. At some point these could be clean from the source, but it is a common problem, and in fact the first streaming project's goal was to clean up bad finagle data!

Ex here there are dubious tags from envoy and probably the shared flag is wrong also. cc @basvanbeek @dio

  {
    "traceId": "978883983d506fa5",
    "id": "978883983d506fa5",
    "kind": "SERVER",
    "name": "localhost:10000",
    "timestamp": 1570523661059232,
    "duration": 127115,
    "localEndpoint": {
      "ipv4": "169.254.65.45"
    },
    "tags": {
      "component": "proxy",
      "downstream_cluster": "-",
      "guid:x-request-id": "d709b78f-583e-988e-b581-25b72c63852d",
      "http.method": "GET",
      "http.protocol": "HTTP/1.1",
      "http.status_code": "200",
      "http.url": "http://www.google.com/",
      "request_size": "0",
      "response_flags": "-",
      "response_size": "67342",
      "upstream_cluster": "service_google",
      "user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36"
    },
    "shared": true
  }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.