WIP / Draft: Prometheusexec receiver working but missing features #1

Nicolas-MacBeth · 2020-06-22T16:08:50Z

This is a work in progress!

It works! Along with my fix in Collector core, I am successfully collecting metrics from 2 different databases at once (Postgres and MySQL)!!! 😄 Everything is implemented, so please go ahead and give feedback! The only thing that may change at a high-level is how I log the subprocesses' output, which we'll talk about in coming meetings. Also, I am currently working on tests, they are coming.

WIP PR for the prometheusexec receiver. Most of the logic is in receiver.go and manager.go. Both config.go files simply define the configuration structs and factory.go is mostly boilerplate code for all receivers in the collector.

… together. Missing functionality

…try-collector-contrib into prometheusexec

draffensperger

Could you create tests for this? I realize it's trickier with processes, but it should be possible - the processes could be simple scripts that just fail, delay for a certain amount of time, etc.

receiver/prometheusexec/receiver.go

receiver/prometheusexec/subprocessmanager/manager.go

Nicolas-MacBeth · 2020-06-22T18:50:40Z

Could you create tests for this? I realize it's trickier with processes, but it should be possible - the processes could be simple scripts that just fail, delay for a certain amount of time, etc.

Yes I will of course be creating tests for all my code, I'll figure out a way for the processes!

…r, added env

…Beth/opentelemetry-collector-contrib into prometheusexec

Small update

typo

…try-collector-contrib into prometheusexec

…tings struct

…Beth/opentelemetry-collector-contrib into prometheusexec

* Revert "Revert "Export spans to hec (#359)" (#375)" This reverts commit 891b1bc. This unreverts the revert of #359, now that the release is out.

The jaeger legacy and kubeletstats receivers were not updated to the latest change in the ReceiverFactoryOld interface open-telemetry/opentelemetry-collector@2f6d603

Bump CPU limits for SAPM traces and OC metrics stability tests to align with the unstable CircleCI environment and avoid sporadic stability tests failures like these: open-telemetry#369 , open-telemetry#354

…379)

receiver/prometheusexec/README.md

receiver/prometheusexec/receiver.go

draffensperger · 2020-07-07T17:21:18Z

receiver/prometheusexec/receiver.go

+	scrapeConfig.JobName = customName
+
+	// This is a default Prometheus scrape config value, which indicates that the scraped metrics can be modified
+	scrapeConfig.HonorLabels = false


Are these things that the user might want to configure? Does the regular Prometheus receiver allow you to configure those?

I can add them! I recall having a conversation with @quentinmit about this and coming to the conclusion to not allow the user to have access to all those settings, and instead going mostly with defaults. Don't mind changing this to enable the user to choose some settings. If I recall correctly we chose not to since the exporters probably all function on the defaults.

receiver/prometheusexec/receiver.go

For the pods that are just created, some stats can be absent. And it causes crashing the otel agent with nil pointer exception.

draffensperger

High level suggestion: I think you should have more testing for this. Specifically:

Unit tests for any parts you can isolate fully
A functional test that actually starts a subprocess (or two) and restarts them with appropriate random ports
Ideally the functional test would be of a subprocess that actually exports a simple hello-world type Prometheus metric (can just run an HTTP server and make it expose a metric via text)

* Add SignalFx demo configuration * collector.yaml * k8s.yaml * Update signalfx-k8s.yaml * Enable Zipkin for Istio Mixer Adapter * Update examples/signalfx/signalfx-collector.yaml Co-authored-by: Paulo Janotti <[email protected]> * Update examples/signalfx/signalfx-k8s.yaml Co-authored-by: Paulo Janotti <[email protected]> * Move to exporter directory Co-authored-by: Paulo Janotti <[email protected]>

receiver/prometheusexec/README.md

receiver/prometheusexec/factory.go

receiver/prometheusexec/receiver.go

quentinmit · 2020-07-07T17:30:08Z

receiver/prometheusexec/receiver.go

+	subprocessConfig.Command = cfg.SubprocessConfig.CommandString
+	subprocessConfig.Port = cfg.SubprocessConfig.Port
+	subprocessConfig.Env = cfg.SubprocessConfig.Env
+	subprocessConfig.CustomName = customName


It's confusing to have two things called config here that have similar but differently named fields - maybe you can reuse the same struct for both?

I renamed the fields to have the same name, but I think the reason I made it this way is that receiver.go (in prometheus_exec) import subprocessmanager already and I can't import prometheus_exec in subprocessmanager to use the same struct since that's a cycle import!

receiver/prometheusexec/receiver.go

receiver/prometheusexec/subprocessmanager/manager.go

Publish builds binaries for all supported platforms with the cross-compile job. Running the build job is redundant for this workflow as the amd64 linux binary is overwritten by cross-compile anyway. Also this can cause CI job to fail if build and cross-compile both persist the same files to CI workspace. However, using cross-compile for publish workflow and only build for regular PR workflow is a bit awkward and adds unnecessary complexity to the CI workflow definition. It is far simpler to just replace build with cross-compile. This means all PR builds will attempt to build binaries for all supported platforms even if we loadtest only linux amd64 right now. I think is a good outcome anyway as PR CI jobs can now catch issues that prevent the collector from building for any of the supported architectures. This also paves the way to enable functional/integration/load testing for all platforms and architectures.

Signed-off-by: Bogdan Drutu <[email protected]>

…ce & added build step for generating Windows MSI (#408)

* Add logging on errors in Runnable Kubeletstats receiver was missing log statements on important errors. * Fix self reported metrics * Fix obsreport calls

…s output

Signed-off-by: Bogdan Drutu <[email protected]>

…try-collector-contrib into prometheusexec

Nicolas-MacBeth added 3 commits June 17, 2020 21:47

First commit with config and factory

4004fe7

WIP: Large part of subprocess manager and config, and how they tie in…

014e508

… together. Missing functionality

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

373e31f

…try-collector-contrib into prometheusexec

Nicolas-MacBeth self-assigned this Jun 22, 2020

draffensperger reviewed Jun 22, 2020

View reviewed changes

Nicolas-MacBeth marked this pull request as draft June 22, 2020 18:52

Nicolas-MacBeth and others added 23 commits June 26, 2020 19:49

Made each subprocess have its own receiver, refined subprocess manage…

1857de8

…r, added env

Readme and folder

ba4caa4

fix small typo

03106c5

Small changes

0dd9ad2

Merge branch 'readme_and_structure' of https://github.com/Nicolas-Mac…

a379ed7

…Beth/opentelemetry-collector-contrib into prometheusexec

Start/stop receiver logic and refactor

3372e15

Readme fixes and updates

9777612

typo fix

f0edac7

Update README.md

1473a3e

Small update

Update README.md

9859111

typo

Update the flags

c6c6344

String templating and random port generation

a185896

Refactor

13f8eaf

small fix

2314f1c

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

48588cf

…try-collector-contrib into prometheusexec

Update to dep v0.5.0

612ff99

context fix for latest version and removed Endpoint from receiver set…

e8f901b

…tings struct

Merge branch 'readme_and_structure' of https://github.com/Nicolas-Mac…

f5956f5

…Beth/opentelemetry-collector-contrib into prometheusexec

Unrevert span export to hec (#399)

b4ad0a4

* Revert "Revert "Export spans to hec (#359)" (#375)" This reverts commit 891b1bc. This unreverts the revert of #359, now that the release is out.

Fix jaeger legacy and kubeletstats metric receivers (#401)

3ae0f00

The jaeger legacy and kubeletstats receivers were not updated to the latest change in the ReceiverFactoryOld interface open-telemetry/opentelemetry-collector@2f6d603

Increase CPU limits for some stability tests (#398)

3df6a48

Bump CPU limits for SAPM traces and OC metrics stability tests to align with the unstable CircleCI environment and avoid sporadic stability tests failures like these: open-telemetry#369 , open-telemetry#354

AWS X-Ray exporter: Only convert SQL information for SQL databases. (#…

4162d00

…379)

Delete legacy (tchannel) Jaeger receiver (#397)

4e6b676

draffensperger reviewed Jul 7, 2020

View reviewed changes

Fix nil pointer exception in kubeletstats receiver (#404)

3d8fc69

For the pods that are just created, some stats can be absent. And it causes crashing the otel agent with nil pointer exception.

draffensperger reviewed Jul 7, 2020

View reviewed changes

quentinmit suggested changes Jul 7, 2020

View reviewed changes

owais and others added 14 commits July 7, 2020 11:34

Update core dependency (#406)

04c2ab5

Signed-off-by: Bogdan Drutu <[email protected]>

Remove zipkin scribe receiver and run go mod tidy everywhere (#410)

86c8ef5

Signed-off-by: Bogdan Drutu <[email protected]>

Updated main function to allow Collector to be run as a Windows Servi…

b34888b

…ce & added build step for generating Windows MSI (#408)

Cleanup (#412)

e3ae869

Move scripts to internal/buildscripts (#414)

5d4921e

Fix kubeletstats logging and self reported metrics (#357)

97e5200

* Add logging on errors in Runnable Kubeletstats receiver was missing log statements on important errors. * Fix self reported metrics * Fix obsreport calls

PR comments fixes, added tcp port check and zap logging for subproces…

016bf63

…s output

Update core dependency (#418)

52e6596

Signed-off-by: Bogdan Drutu <[email protected]>

Correctly obtain machine-type info from gce metadata (#395)

00d9077

rm go.sum; go mod tidy (#419)

15e709d

Signed-off-by: Bogdan Drutu <[email protected]>

Move ComponentDocs test to internal/common and go mod tidy (#415)

d2861ad

change the subprocess output to use reader instead of scanner

53624d6

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

da718db

…try-collector-contrib into prometheusexec

Nicolas-MacBeth closed this Jul 15, 2020

WIP / Draft: Prometheusexec receiver working but missing features #1

WIP / Draft: Prometheusexec receiver working but missing features #1

Uh oh!

Conversation

Nicolas-MacBeth commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

draffensperger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nicolas-MacBeth commented Jun 22, 2020

Uh oh!

Uh oh!

Uh oh!

draffensperger Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Nicolas-MacBeth Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

draffensperger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quentinmit Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Nicolas-MacBeth Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Nicolas-MacBeth commented Jun 22, 2020 •

edited

Loading

Nicolas-MacBeth Jul 8, 2020 •

edited

Loading