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

Support for plugin span filters #2072

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@afalko
Contributor

afalko commented Jun 1, 2018

This is one of the requirements for the architecture described here: https://docs.google.com/document/d/11_MB69A6d_9_N5ClxVcUhKJRMvVI18voV5cGv1Ji5oc/edit?usp=sharing

In addition to SpanFilters we'd like to introduce HttpFilters so that we can respond to things found in HttpHeaders. That would be a follow up PR.

This is an enrichment filter we plan to deploy to production as our first filter: https://github.com/afalko/zipkin-generic-enrichment-filter . I'm happy to move this filter to openzipkin project if requested.

Additional filters that we plan to implement:

ServiceTagMappingFilter
This filter would take in a static configuration where services claim tag and annotation identifiers. If another service comes in and tries to use a reserved tag, it will get a 409 (conflict) error from our server. An alternative scheme can be developed whereby annotations and tags are namespaced, the drawback of that would be an increase in storage requirements.
Another way to implement this filter is to remove conflicting tags, but not drop the message. In that case, the regular 202 http message is sent back to the caller.

GDPRFilters
These filters depend on the company and the nature of the data processed by the apps of that company. There might be cases where the implementer might want to enrich spans or replace the data within the spans. These would likely never have any use if they are open sourced. They are not in the minimum viable change path.

An additional example filter that does blocking can be found here: https://github.com/afalko/ExampleSpanFilter

  • [] Utests for failure cases

@afalko afalko referenced this pull request Jun 1, 2018

Open

Kafka storage backend #2060

0 of 3 tasks complete
@adriancole

thanks for spiking this. I think filter might not be the best word as it seems this is also mutating data.. maybe put in the PR the use cases this is for?

@@ -283,6 +283,8 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
<configuration>
<!-- This forces Spring Boot to use the PropertiesLauncher for startup, enabling loader.path -->

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

any impact to this? especially for existing modules? cc @zeagord cc @shakuzen

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

@rmichela helped me with all of the Spring stuff, so he might be able to go into more detail on this.

@@ -56,10 +61,10 @@
private HttpHandler next;
@Autowired ZipkinHttpCollector(StorageComponent storage, CollectorSampler sampler,
CollectorMetrics metrics) {
CollectorMetrics metrics, Optional<List<SpanFilter>> filters) {

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

we don't have to inject optional right? can't we just get an empty list?

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

I had this with @Nullable before, but I kind of like Optional from a readability standpoint. Happy to change it back. @rmichela, is it better to use Optional or @Nullable?

This comment has been minimized.

@rmichela

rmichela Jun 1, 2018

The Optional<T> is required so Spring knows the constructor injected filters parameter is not required. Without it, the bean graph will fail to load at startup when no SpanFilter plugin implementations are registered with Spring.

https://stackoverflow.com/a/42135423

@@ -74,23 +78,34 @@ public Builder sampler(CollectorSampler sampler) {
return this;
}
/** @see {@link CollectorComponent.Builder#filters(List<SpanFilter>)} */
public Builder filters(List<SpanFilter> filters) {
logger.info("Total filters loaded: " + filters.size());

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

remove this.. as we don't log builder invocations. probably can accomplish the same at higher abstraction ex /autoconfig endpoint

super(message);
this.httpCode = httpCode;
}

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

this seems leaky abstraction as it isnt necessarily http but has http code. also extending throwable is a bit weird. maybe use a built-in exception or document how this is used as a signal. we don't currently define custom exceptions anywhere else in the codebase

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

I'll look around from suitable built-ins. I'll add better documentation too. The reason I extended Throwable was because the error call takes a throwable: https://github.com/openzipkin/zipkin/pull/2072/files#diff-1b30356bd2b01dbbfd9152c17a1ac4baR148

In theory, I could copy what you did in that same function: message.startsWith("Cannot store"), but that isn't strongly typed and requires an implementor to have to be careful about their error strings. I wanted to avoid that drawback and make it easier on filter implementors. Happy to send another PR to clean replace message.startsWith.

* implementor wants to produce custom return codes back to the user.
* @param spans
* @param callback
*/

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

hmm are you hinting there is a partial success here? ex you can return without exception?

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

If the implementor so chooses they could consider ingesting all spans, but throwing an error. I think that would be a rare case, but I can't think of reasons to get in someone's way of doing that; nor can I think of a strong reason to go out of our way to support that very well. Do you want me to have the call of the interface stop if exception is added to callback?

List<S> filterSpans(List<S> spans, Callback<Void> callback) {
// These should never be processed in parallel.
// If you need parallel processing put that into your specific filter implementation.
boolean isV1Span = spanType.getType().equals(Span.class);

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

we won't be filtering on two types. you can assume later it will be only zipkin2.span

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

Ahh, that's good news; I won't need the guava token type once that is that case. In terms of "later", do you want me to make this only supported for v2 as part of this PR?

@@ -60,6 +60,12 @@
<version>2.8.2</version>
</dependency>

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

heh no way jose ;) gave can't be a core dep as it conflicts

This comment has been minimized.

@afalko

afalko Jun 1, 2018

Contributor

I only need it for TokenType to determine v1 vs. v2. If zipkin2.Span is that only thing that is supported in the next week or two, then I can remove this. Otherwise, I need suggestions :).

README.md Outdated
Here is an example enrichment filter that allows you to inject a tag to your spans from environment passed to Zipkin:
[zipkin-generic-enrichment-filter](https://github.com/afalko/zipkin-generic-enrichment-filter)
### Zipkin Http Filters

This comment has been minimized.

@adriancole

adriancole Jun 1, 2018

Contributor

please remove forward refs to http filters which I am not sure what they will be. makes the change easier to pin down.

@afalko

This comment has been minimized.

Contributor

afalko commented Jun 1, 2018

Most of our use cases are outlined in the doc I linked, but I'll paste them in the PR description shortly. Other names I can think of are SpanProcessors, SpanManipulators, SpanManglers. Open to other suggestions or votes; naming is hard :)

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 3, 2018

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 3, 2018

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 3, 2018

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 7, 2018

ok this can be redone over the new zipkin-collector library, which is an abstract class so no api break :P

I'd recommend just using the Adjuster type as we did some work on this for the sparkstreaming api.

https://github.com/openzipkin-attic/zipkin-sparkstreaming/blob/master/sparkstreaming/src/main/java/zipkin/sparkstreaming/Adjuster.java

The only difference is that it is unlikely the spans will be in the same trace..

Adjustment should be a pure function, meaning it shouldn't throw exceptions. I think we should start with this assumption and move forward from there.. basically in hindsight I think we can get most mileage by taking some of the work @naoman helped with and make it into the core collector library along with your input too.. two for one that way :P

int code;
if (e instanceof FilterActivatedException) {
FilterActivatedException filterException = (FilterActivatedException)e;
code = filterException.getHttpCode();

This comment has been minimized.

@afalko

afalko Jun 12, 2018

Contributor

@adriancole I tried to find a built-in exception class that carries something like http code, but unfortunately there isn't one that I can bring in without big dependencies.

These two are the most commonly used:

  1. http://hc.apache.org/httpclient-3.x/apidocs/org/apache/commons/httpclient/HttpException.html
  2. https://docs.oracle.com/javase/7/docs/api/javax/xml/ws/http/HTTPException.html

I still think the most reasonable option is to have the custom exception. Without this, a filter implementor cannot adjust the http code emitted if there is an error, which would make for a bad user experience.

This comment has been minimized.

@afalko

afalko Jun 13, 2018

Contributor

I take part of this back. The latter exception will suffice for the error code and is bolted in java. The only drawback is that it doesn't provide a way to set a custom message.

@afalko

This comment has been minimized.

Contributor

afalko commented Jun 13, 2018

@adriancole I took a closer look at adjuster and I think is too specific for what we are trying to do. I can imagine a filter implementation having that adjuster class internal to it and it would be invoked within the process implementation. I'm meeting with @naoman next week to find out more about his work and if my soon-to-be-revised PR will work for his use-cases.

@afalko afalko force-pushed the afalko:filters branch 2 times, most recently from e07d890 to 258edfb Jun 13, 2018

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 13, 2018

@adriancole

This comment has been minimized.

Contributor

adriancole commented Jun 13, 2018

Support for filters
Thanks to rmichela for helping with spring auto-loading of classes

@afalko afalko force-pushed the afalko:filters branch from 258edfb to f67966c Jun 13, 2018

@afalko

This comment has been minimized.

Contributor

afalko commented Jun 13, 2018

I've rebased on latest master and removed the custom exception. I also pass CollectorMetrics through into the SpanFilter interface. I've added a utest that drops spans too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment