Add tenant ID processor #12
Conversation
|
Thanks @pavolloffay, nice find. Although I would hold on with this PR as it does not solve our use case ATM. |
go.mod
Outdated
| @@ -7,6 +7,7 @@ require ( | |||
| github.com/go-sql-driver/mysql v1.5.0 // indirect | |||
| github.com/golangci/golangci-lint v1.31.0 // indirect | |||
| github.com/google/addlicense v0.0.0-20200622132530-df58acafd6d5 // indirect | |||
| github.com/jaegertracing/jaeger v1.21.0 | |||
There was a problem hiding this comment.
I am very reluctant to add this kind of things just for testing (disclaimer: this is a dependency/test argument rather than a library specific one). Also I don't see the need for this given we have a generic test for GRPC without the need of adding a new library: https://github.com/hypertrace/hypertrace-collector/pull/12/files#diff-dc84efdcda988ef18ddc3ae41e3f99d20f5b172fd77cea30016087f334cd9e70R63. Could you elaborate more on the benefit of this?
There was a problem hiding this comment.
I can remove the Jaeger test. The dependency on Jaeger will be pulled in anyways since the jaeger receiver is in the standard components.
There was a problem hiding this comment.
It is alright that dependency will be pulled by a dependency as long as we keep our list of dependencies minimal.
|
|
||
| import ( | ||
| "context" | ||
| "go.opencensus.io/stats/view" |
There was a problem hiding this comment.
if we want to use metrics then yes. I would stick with metrics API that the upstream uses, there is plan to migrate but who knows when open-telemetry/opentelemetry-collector#816
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
force-pushed
the
tenantid-processor2
branch
from
127c1bb to
ce1916a
Compare
What use-case are you refering to? This PR migrates a processor from the old traceable OC service. We need this functionality here as well. There is nothing controversial in the PR to put it on hold. The support for Jaeger thrift will be added in a separate PR. I don't want to keep open PRs and keep rebasing. |
There was a problem hiding this comment.
Thanks @pavolloffay, nice find. Although I would hold on with this PR as it does not solve our use case ATM.
What use-case are you refering to? This PR migrates a processor from the old traceable OC service. We need this functionality here as well. There is nothing controversial in the PR to put it on hold. The support for Jaeger thrift will be added in a separate PR. I don't want to keep open PRs and keep rebasing.
I think it makes sense to get this in as is, because as Pavol states, this is needed functionality. We will also need to support jaeger thrift for compatibility, but that will be a later PR.
| tenantIDHeaders := md.Get(p.tenantIDHeaderName) | ||
| if len(tenantIDHeaders) == 0 { | ||
| return traces, nil | ||
| } else if len(tenantIDHeaders) > 1 { |
There was a problem hiding this comment.
is there ever a case where this would be valid? I'm inclined to err on the side of caution and
just drop spans which have multiple tenant header values.
There was a problem hiding this comment.
TBH I don't know, it's probably safer to return an error in this case. Every user should have assigned a single tenant.
Signed-off-by: Pavol Loffay <[email protected]>
|
@jcchavezs @davexroth PR updated. |
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
I don't see anything controversial in this PR. My main concern is that this does not address our main use cases (report over jaeger and zipkin endpoint) and the more processors we add at this stage, the more we have to maintain towards breaking changes from otel. If we delay the merge we only need to do the update the processors in here. Remember in this PR we mostly add new code and that won't be affected by rebases most likely cc @davexroth @pavolloffay |
Signed-off-by: Pavol Loffay <[email protected]>
|
This processor is not needed for Zipkin and as it is stated above the Jaeger support will be added in upstream and here in a separate PR (e.g. as a test at least). This processor will be used with OTLP (going forward) and Jaeger Thrift over HTTP for existing deployments. At traceable Zipkin is used only to report data from agents to a local collector in the customer cluster. |
This PR adds tenant ID processor that adds tenant header as an attribute to every span. If the header is missing an error is returned.
The headers are obtained from context, this works well for grpc receivers (see test with OTLP and Jaeger). Accessing headers from other receivers (e.g. HTTP) is tracked under open-telemetry/opentelemetry-collector#2401.