Add prometheus_simple receiver #184

asuresh4 · 2020-04-23T20:11:01Z

Description

The prometheus_simple receiver is a wrapper around the prometheus receiver.
This receiver provides a simple configuration interface to configure the prometheus
receiver to scrape metrics from a single target. This receiver would provide the receiver_creator a simplified configuration interface to collect prometheus metrics.

An Example config,

    receivers:
      prometheus_simple:
        endpoint: "172.17.0.5:9153"

The receiver can also be configured to use a Pod service account when running in a Kubernetes environment using use_service_account.

Testing

Unit tests
Manual testing with SignalFx exporter

Documentation

https://github.com/signalfx/opentelemetry-collector-contrib/blob/simple-prometheus/receiver/simpleprometheusreceiver/README.md

jrcamp · 2020-04-24T20:14:47Z

receiver/simpleprometheusreceiver/config.go

+	// Whether or not to use pod service account to authenticate.
+	UseServiceAccount bool `mapstructure:"use_service_account"`
+	// TLS configs in case connection is over HTTPS
+	TLSCredentials *tlsCredentials `mapstructure:"tls_credentials"`


I think I might just call the config value tls as they're not necessarily credentials. For example you might set ca_file but not use cert_file and key_file for auth.

What does this look like in config if you want to enable TLS but with just the defaults? If you do:

receivers: prometheus_simple: tls_config:

I think tls will be nil so not configured. But if you did:

receivers: prometheus_simple: tls_config: {}

TLS would be non-nil with the default zero values. Seems like it might be easy to mess up. 🤔

Since paths to the cert, key and the CA cert are all empty an empty TLS config would fail authentication right? Also, would it be a valid use case to have these be the empty default values?

If it's empty it should use the system CA certs.

Looks like they avoid this problem by having TLS explicitly enabled or disabled with scheme.

Oh ok. I just looked at how prometheus handles these values and from first look, it does not seem like it picks up the system CA certs. Will dig a bit deeper and get back.

I think it'd end up using the default which is system default CA:

https://golang.org/src/crypto/tls/common.go?s=16110:25304#L526

// RootCAs defines the set of root certificate authorities // that clients use when verifying server certificates. // If RootCAs is nil, TLS uses the host's root CA set. RootCAs *x509.CertPool

receiver/simpleprometheusreceiver/config.go

tigrannajaryan · 2020-04-27T21:21:48Z

@asuresh4 please resolve the conflict.

tigrannajaryan

LGTM, I already reviewed this code on an internal fork.

jrcamp

What was the outcome of #184 (comment)?

asuresh4 · 2020-04-28T14:41:03Z

What was the outcome of #184 (comment)?

tls_credentials was renamed to tls_config.

b3422ae

jrcamp · 2020-04-28T14:52:02Z

What about the next comment #184 (comment)?

asuresh4 · 2020-04-28T16:28:03Z

What about the next comment #184 (comment)?

Based on the discussion we had, I tried the following configs,

  prometheus_simple:
    endpoint: "192.168.64.17:10250"
    tls_config: {}

and

  prometheus_simple:
    endpoint: "192.168.64.17:10250"
    tls_config:

In both cases config.tlsConfig == nil equates to true. Which means it wouldn't be possible to use https with just the default system certs unless explicitly ca_file is specified in the config.

To be able to support https using default system CA certs, we would need to have another config to indicate whether or not to use TLS. For example, if the config was

  prometheus_simple:
    endpoint: "192.168.64.17:10250"
    scheme: https (or something equivalent)
    tls_config:

the client would use TLS with default system CA certs in this case.

@tigrannajaryan @jrcamp

jrcamp · 2020-04-28T16:32:42Z

scheme: https (or something equivalent)

Would suggest making this tls_enabled: true|false so that it is protocol-agnostic and can be included in a shared TLS config struct.

tigrannajaryan · 2020-04-28T18:06:28Z

@jrcamp @asuresh4 so just to clarify. This is about TLS settings for a client, right? We previously had TLSCredentials for receivers, which supposedly were TLS servers.

For clients we can use a different config approach as you suggested. We can have 2 sibling keys: tls_enabled which is boolean and tls_config which contains subkeys with CA file path, etc.

I think this is fine. We don't have to use the same config struct for TLS clients and for TLS servers (which is what existing TLSCredentials for).

Sorry, I may have mislead you on this.

tigrannajaryan · 2020-04-28T18:09:36Z

receiver/simpleprometheusreceiver/README.md

+        ca_file: "/path/to/ca"
+        cert_file: "/path/to/cert"
+        key_file: "/path/to/key"
+        insecure_skip_verify: true


I think this is no longer correct. This needs to be under tls_config.

The TLS options and the ones in httpConfig are embedded in the receiver config so these would still be at the top level.

Oh, I didn't notice tls_config was removed. I was thinking we'd keep it but just add tls_enabled beside it.

Ah yes, looking back at the example I gave above, I realized that's what we discussed too. Updated.

tigrannajaryan

LGTM

### Description The `prometheus_simple` receiver is a wrapper around the [prometheus receiver](https://github.com/open-telemetry/opentelemetry-collector/tree/master/receiver/prometheusreceiver). This receiver provides a simple configuration interface to configure the prometheus receiver to scrape metrics from a single target. This receiver would provide the `receiver_creator` a simplified configuration interface to collect prometheus metrics. An Example config, ```yaml receivers: prometheus_simple: endpoint: "172.17.0.5:9153" ``` The receiver can also be configured to use a Pod service account when running in a Kubernetes environment using `use_service_account`. ### Testing Unit tests Manual testing with SignalFx exporter ### Documentation https://github.com/signalfx/opentelemetry-collector-contrib/blob/simple-prometheus/receiver/simpleprometheusreceiver/README.md

There is only one config now, so no need to have version number in the file name.

Also fixes example module paths so that they use the vanity URL instead of the github URL. [Closes #184]

Update redis instrumentation to follow semantic conventions

…y#184) * Add Neuron monitor scraper to scrape metrics from NeuronMonitor --------- Co-authored-by: Hyunsoo Kim <hsookim@amazon.com> Co-authored-by: Aditya Purang <puranga@amazon.com>

asuresh4 requested review from bogdandrutu, flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners April 23, 2020 20:11

jrcamp reviewed Apr 24, 2020

View reviewed changes

asuresh4 added 3 commits April 27, 2020 10:25

Add prometheus_simple receiver

00bde68

Address feedback

b3422ae

Incorporate latest updates

3f97f68

tigrannajaryan approved these changes Apr 27, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into simple-prometheus

7484afd

jrcamp suggested changes Apr 28, 2020

View reviewed changes

Handle tlsConfig default behavior

36d9b64

asuresh4 requested a review from tigrannajaryan April 28, 2020 17:50

jrcamp approved these changes Apr 28, 2020

View reviewed changes

tigrannajaryan reviewed Apr 28, 2020

View reviewed changes

Don't embed tls_config in httpConfig

2bfa92b

jrcamp approved these changes Apr 28, 2020

View reviewed changes

tigrannajaryan reviewed Apr 28, 2020

View reviewed changes

tigrannajaryan approved these changes Apr 28, 2020

View reviewed changes

tigrannajaryan merged commit e657c62 into open-telemetry:master Apr 28, 2020

asuresh4 deleted the simple-prometheus branch April 29, 2020 20:16

asuresh4 mentioned this pull request May 1, 2020

Move HTTP config to common internal package #198

Closed

asuresh4 mentioned this pull request May 28, 2020

REQUEST: New membership for asuresh4 open-telemetry/community#358

Closed

6 tasks

mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020

Rename configv2 to config (#184)

4169555

There is only one config now, so no need to have version number in the file name.

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

Namespace import path under "/otel" (#274)

15bfc5b

Also fixes example module paths so that they use the vanity URL instead of the github URL. [Closes #184]

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Merge pull request #184 from lonewolf3739/redis-semantic-conv

3c207e3

Update redis instrumentation to follow semantic conventions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prometheus_simple receiver #184

Add prometheus_simple receiver #184

asuresh4 commented Apr 23, 2020

jrcamp Apr 24, 2020

jrcamp Apr 24, 2020 •

edited

asuresh4 Apr 24, 2020

jrcamp Apr 24, 2020

jrcamp Apr 24, 2020

asuresh4 Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan commented Apr 27, 2020

tigrannajaryan left a comment

jrcamp left a comment

asuresh4 commented Apr 28, 2020

jrcamp commented Apr 28, 2020

asuresh4 commented Apr 28, 2020

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

tigrannajaryan Apr 28, 2020

asuresh4 Apr 28, 2020

jrcamp Apr 28, 2020

asuresh4 Apr 28, 2020

tigrannajaryan left a comment

Add prometheus_simple receiver #184

Add prometheus_simple receiver #184

Conversation

asuresh4 commented Apr 23, 2020

Description

Testing

Documentation

Choose a reason for hiding this comment

jrcamp Apr 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Apr 27, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

jrcamp left a comment

Choose a reason for hiding this comment

asuresh4 commented Apr 28, 2020

jrcamp commented Apr 28, 2020

asuresh4 commented Apr 28, 2020

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

jrcamp Apr 24, 2020 •

edited