Added client package to help identify RPC/HTTP clients #326

owais · 2019-09-05T16:15:04Z

Client package will help identify clients connecting to different
receivers and enable usecases where processors might depend on some
information about the client. For example, a rate-limiting processor
might need some information about clients to figure out when and how to
rate-limit. Also, the upcoming kubernetes processor depends on this as
it needs the IP address of the clients to properly identify resources.

Submitting this as an independent PR as it is generic enough and not
tied to any single processor or usecase.

This PR adds support to the OC, Zipkin and Jaeger GRPC receivers. I'll implement the
same feature for older jaeger receivers in a follow up PR.

songy23 · 2019-09-05T17:22:37Z

*** directories without *_test.go files:
./client
error: at least one *_test.go file must be in all directories with go files so that they are counted for code coverage
       if no tests are possible for a package (e.g. it only defines types), create empty_test.go
Makefile:60: recipe for target 'test-with-cover' failed
make: *** [test-with-cover] Error 1

Please either add a test file or a .nocover.

codecov-io · 2019-09-05T18:14:51Z

Codecov Report

Merging #326 into master will increase coverage by 0.02%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
+ Coverage   75.65%   75.68%   +0.02%     
==========================================
  Files         120      121       +1     
  Lines        7354     7385      +31     
==========================================
+ Hits         5564     5589      +25     
- Misses       1523     1528       +5     
- Partials      267      268       +1

Impacted Files	Coverage Δ
receiver/zipkinreceiver/trace_receiver.go	`62.02% <0%> (-0.44%)`	⬇️
receiver/jaegerreceiver/trace_receiver.go	`80.17% <100%> (+0.17%)`	⬆️
receiver/opencensusreceiver/octrace/opencensus.go	`72.36% <100%> (+1.13%)`	⬆️
client/client.go	`83.33% <83.33%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b468dee...7d7e1c8. Read the comment docs.

owais · 2019-09-05T23:42:18Z

@songy23 updated.

tigrannajaryan

LGTM. I had already reviewed this in our private fork.

bogdandrutu · 2019-09-06T01:28:23Z

Quick question:

What is the interaction between this and Node?
Why propagated it via ctx and not part of the [Trace|Metrics]Data?
How does a batching/tailbasesampling set the client for the next processors?

owais · 2019-09-06T16:29:20Z

The idea is to pass it through Resource/Node using a processor. This just lays the foundations for that by just plumbing through the client info to the processors. I was thinking a "Resource Detector" processor could use this information from the context to add relevant info to the span/batch resource and/or node. So the end goal is to use resource/node. This is just a stepping stone towards that.

Alternatively, each receiver could add this info directly to the resource/node but I think the tagging can be a bit nuanced. We might need configurable checks and filters to control the behaviour. For example, we shouldn't tag the resource with the IP if the sender was a Jaeger agent. With a common resource tagger processor, we can configure it to detect and ignore such clients.

Also the receivers so far have not been modifying the incoming data and processors have dealt with that mostly.

That said, I think we can explore adding this info directly to the resource/node objects inside receivers. Another alternative could be an implicit processor that sits between the receiver(s) and the rest of the pipeline.

owais · 2019-09-06T16:29:55Z

(I'm flying to India today so might not be able to reply in time)

ccaraman · 2019-09-09T22:45:51Z

receiver/opencensusreceiver/octrace/opencensus.go

@@ -189,8 +190,13 @@ func (rw *receiverWorker) export(longLivedCtx context.Context, tracedata *consum
 		return
 	}

+	ctx := context.Background()
+	if c, ok := client.FromGRPC(longLivedCtx); ok {
+		ctx = client.NewContext(ctx, c)


Why can't the long live ctx be used as an input here?

longLivedCtx is bound to the stream handler loop and remains active for the lifetime of the connection. The new context is used per request (incoming batch) to trace handling of each batch as a single independent request. This is not a new change and continues to work as before.

owais · 2020-01-08T14:52:07Z

@bogdandrutu @pjanotti @songy23 can you please take another look?

tigrannajaryan · 2020-01-08T23:59:17Z

@open-telemetry/collector-approvers please review. This is blocking another PR.

Client package will help identify clients connecting to different receivers and enable usecases where processors might depend on some information about the client. For example, a rate-limiting processor might need some information about clients to figure out when and how to rate-limit. Also, the upcoming kubernetes processor depends on this as it needs the IP address of the clients to properly identify resources. Submitting this as an independent PR as it is generic enough and not tied to any single processor or usecase. This PR adds support to the OC, Zipkin and Jaeger GRPC receivers. I'll implement the same feature for older jaeger receivers in a follow up PR.

pjanotti

Change LGTM. One question: why not also adding to Jaeger other than gRPC?

owais · 2020-01-15T21:24:53Z

Jaeger is planned. I just didn't find a straightforward way to do it back then with existing jaeger receivers. However it is mitigated because Jaeger agents/SDKs almost always add an IP tag on the process which processors can fall back on.

* Add MetricAggregator.Merge() implementations * Update from feedback * Type * Ckpt * Ckpt * Add push controller * Ckpt * Add aggregator interfaces, stdout encoder * Modify basic main.go * Main is working * Batch stdout output * Sum udpate * Rename stdout * Add stateless/stateful Batcher options * Undo a for-loop in the example, remove a done TODO * Update imports * Add note * Rename defaultkeys * Support variable label encoder to speed OpenMetrics/Statsd export * Lint * Checkpoint * Checkpoint * Doc * Precommit/lint * Simplify Aggregator API * Record->Identifier * Remove export.Record a.k.a. Identifier * Checkpoint * Propagate errors to the SDK, remove a bunch of 'TODO warn' * Checkpoint * Introduce export.Labels * Comments in export/metric.go * Comment * More merge * More doc * Complete example * Lint fixes * Add a testable example * Lint * Dogstats * Let Export return an error * Checkpoint * add a basic stdout exporter test * Add measure test; fix aggregator APIs * Use JSON numbers, not strings * Test stdout exporter error * Add a test for the call to RangeTest * Add error handler API to improve correctness test; return errors from RecordOne * Undo the previous -- do not expose errors * Add simple selector variations, test * Repair examples * Test push controller error handling * Add SDK label encoder tests * Add a defaultkeys batcher test * Add an ungrouped batcher test * Lint new tests * Respond to krnowak's feedback * Checkpoint * Funciontal example using unixgram * Tidy the example * Add a packet-split test * More tests * Undo comment * Use concrete receivers for export records and labels, since the constructors return structs not pointers * Bug fix for stateful batchers; clone an aggregator for long term storage * Remove TODO addressed in open-telemetry#318 * Add errors to all aggregator interfaces * Handle ErrNoLastValue case in stdout exporter * Move aggregator API into sdk/export/metric/aggregator * Update all aggregator exported-method comments * Document the aggregator APIs * More aggregator comments * Add multiple updates to the ungrouped test * Fixes for feedback from Gustavo and Liz * Producer->CheckpointSet; add FinishedCollection * Process takes an export.Record * ReadCheckpoint->CheckpointSet * EncodeLabels->Encode * Format a better inconsistent type error; add more aggregator API tests * More RangeTest test coverage * Make benbjohnson/clock a test-only dependency * Handle ErrNoLastValue in stress_test * Update comments; use a pipe vs a unix socket in the example test * Update test * Spelling * Typo fix * Rename DefaultLabelEncoder to NewDefaultLabelEncoder for clarity * Rename DefaultLabelEncoder to NewDefaultLabelEncoder for clarity * Test different adapters; add ForceEncode to statsd label encoder

owais requested review from bogdandrutu, flands, pjanotti, songy23 and tigrannajaryan as code owners September 5, 2019 16:15

tigrannajaryan approved these changes Sep 6, 2019

View reviewed changes

owais mentioned this pull request Sep 6, 2019

REQUEST: New membership for owais open-telemetry/community#146

Closed

6 tasks

owais mentioned this pull request Sep 7, 2019

Nominating @owais as approver for Agent and Collector open-telemetry/community#172

Merged

ccaraman reviewed Sep 9, 2019

View reviewed changes

owais requested a review from rghetia as a code owner January 8, 2020 14:48

owais mentioned this pull request Jan 8, 2020

Porting k8s processor from the Omnition distribution open-telemetry/opentelemetry-collector-contrib#92

Merged

pjanotti approved these changes Jan 15, 2020

View reviewed changes

tigrannajaryan merged commit 264426a into open-telemetry:master Jan 15, 2020

owais deleted the add-client-package branch January 15, 2020 23:58

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Fix main branch links (open-telemetry#326)

2156247

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

Remove go dependancies from dependabot config (open-telemetry#326)

92e56e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added client package to help identify RPC/HTTP clients #326

Added client package to help identify RPC/HTTP clients #326

owais commented Sep 5, 2019 •

edited

Loading

songy23 commented Sep 5, 2019

codecov-io commented Sep 5, 2019 •

edited

Loading

owais commented Sep 5, 2019

tigrannajaryan left a comment

bogdandrutu commented Sep 6, 2019

owais commented Sep 6, 2019

owais commented Sep 6, 2019

ccaraman Sep 9, 2019

owais Jan 8, 2020

owais commented Jan 8, 2020

tigrannajaryan commented Jan 8, 2020

pjanotti left a comment

owais commented Jan 15, 2020

Added client package to help identify RPC/HTTP clients #326

Added client package to help identify RPC/HTTP clients #326

Conversation

owais commented Sep 5, 2019 • edited Loading

songy23 commented Sep 5, 2019

codecov-io commented Sep 5, 2019 • edited Loading

Codecov Report

owais commented Sep 5, 2019

tigrannajaryan left a comment

Choose a reason for hiding this comment

bogdandrutu commented Sep 6, 2019

owais commented Sep 6, 2019

owais commented Sep 6, 2019

ccaraman Sep 9, 2019

Choose a reason for hiding this comment

owais Jan 8, 2020

Choose a reason for hiding this comment

owais commented Jan 8, 2020

tigrannajaryan commented Jan 8, 2020

pjanotti left a comment

Choose a reason for hiding this comment

owais commented Jan 15, 2020

owais commented Sep 5, 2019 •

edited

Loading

codecov-io commented Sep 5, 2019 •

edited

Loading