Docker Stats Receiver #495
Conversation
Codecov Report
@@ Coverage Diff @@
## master #495 +/- ##
==========================================
+ Coverage 88.14% 88.45% +0.30%
==========================================
Files 236 242 +6
Lines 12575 13040 +465
==========================================
+ Hits 11084 11534 +450
- Misses 1137 1147 +10
- Partials 354 359 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It will be almost impossible for one of the maintainer to review 5000+ lines. Consider to split this PR into more readable PRs, as a rule of thumb 500 lines is considered a large PR
While in development consider to not add it to the components.go in the service.
|
@bogdandrutu not to split hairs, but after removing an undesired metadata file and taking go.mod/sum out of the picture it's a more manageable ~3k lines, a lot of which is test data. Still a large PR, but it's a fairly major feature addition. Will break up into more reasonable commits for each component to help determine if separate PRs are necessary. Common metric creation utils and validation helpers would dramatically cut back on the scope of additions as well. I plan on adding these in a future effort, unrelated to this feature. |
|
Ryan, in the interest of breaking this PR into smaller chunks, I'm wondering if there's a seam that we could find on which to split these changes. Like, if I understand correctly, this receiver has one part (A) that reads the current containers and listens for changes, and another part (B) that fetches container stats and converts them to metrics on a schedule. Is this an accurate assessment? Would it be helpful to create a PR that just does A for now? And in support of that effort, would it be easier to handle the distinct functions (A and B) in separate classes? |
|
A) listens for container events from the docker daemon and maintains an internal store for containers that should be queried for stats. B) iterates over the internal store on an interval, fetches stats and converts them to metrics. I think the simplest functional PR would consist of B, and a subsequent one would introduce the event handling. However, it's not clear to me that sacrificing the initial dynamic functionality would be worth the feature penalty. Happy to break apart if it would make others' efforts easier overall though. |
|
Sounds good to me. Not sure how functional it has to be while in development though, at least not until the receiver is registered in components.go, but if it is functional, then I suppose that's more Agile. :-) |
|
@pmcollins I've taken out the dynamic container updates, but left some of the scaffolding it will use in the interest of not spending too much effort on things that will immediately be undone. |
|
Can you rebase? |
|
@pmcollins sorry, not sure why my changes didn't land earlier. (actually) Added the lock for container removal and adopted the new factory helpers. |
|
|
||
| // Queries inspect api and returns *ContainerJSON and true when container should be queried for stats, | ||
| // nil and false otherwise. | ||
| func (dc *DockerClient) inspectedContainerIsOfInterest(ctx context.Context, cid string) (*dtypes.ContainerJSON, bool) { |
There was a problem hiding this comment.
I'm wondering if inspectedContainerIsOfInterest, shouldBeExcluded and excludedImageMatcher should all go into a separate filter type/class.
There was a problem hiding this comment.
I'm not sure I follow or agree atm. inspectedContainerIsOfInterest provides a critical docker client containerInspect call, which belongs in the core type imo. excludedImageMatcher is a StringMatcher, an already separate type, and the associated shouldBeExcluded provide a nil check only applicable to dockerClient.
There was a problem hiding this comment.
Ok, that makes sense. Is shouldBeExcluded needed though -- it looks like excludedImageMatcher can't be nil.
There was a problem hiding this comment.
it's a pointer to a StringMatcher so it can be right, but isn't in practice thus far? I'm not sure of what is idiomatic but this seems like a straightforward nil check.
|
@jrcamp, @keitwb, @asuresh4, @bogdandrutu any chance you can take (another) look? |
Thanks, @tigrannajaryan, added in #917 |
The dependency was referring to non-existing version. This was building fine in this repo because the dependency is replaced but was causing problem in other repos which consume the testbed.
* Add zipkin exporter The zipkin exporter implements the SpanBatcher interface. It follows the current-at-the-time-of-writing document about conversion from OpenTelemetry span data to Zipkin spans. Which means that endpoint information is not yet filled. * Fix typo in docs * Add a zipkin example This sends span information to a locally running zipkin collector. Currently I have a problem getting the collector to show me the spans after accepting them with HTTP 202. Not sure if this is because of missing endpoint information. * Make gitignore consistent The fixed paths should be prefixed with a slash. The "relative" paths mean that git will ignore all the files that end with the path. * Add tests for zipkin exporter
Description: Adds a new
docker_statsreceiver that fetches container stats on a configured interval. Functionality is largely ported from the SignalFx Smart Agent docker container stats monitor: https://github.com/signalfx/signalfx-agent/blob/master/docs/monitors/docker-container-stats.mdTesting: Provides unit and integration tests.
Documentation: Readme section added.