[exporter/tanzuobservability] Make metrics stanza optional in config. #9098

keep94 · 2022-04-05T21:49:07Z

Description:
Fixed bug. The collector failed to start if the tanzuobservability exporter was configured with traces only and not metrics and the traces endpoint had anything other than localhost. This is a bug because traces came first, and metrics came later. If a user has only traces configured for tanzuobservability exporter, they should be able to start the collector even if they have upgraded to a version that supports metrics too.

To fix, I made the Metrics field in our config struct be a pointer field instead of a struct field, so that it will be nil if it is missing. If metrics aren't configured in the config file, we don't send any metrics.

Link to tracking Issue: Not available to public.

Testing:.

Unit testing.
Test that I can start the collector when the traces endpoint is http://192.168.XX.XX:30001 and metrics are not configured and that traces are actually sent.
Test that metrics get sent when the metrics endpoint is set to http://192.168.XX.XX:2878.

Documentation: Changes to README.md

exporter/tanzuobservabilityexporter/exporter.go

exporter/tanzuobservabilityexporter/metrics_exporter.go

exporter/tanzuobservabilityexporter/exporter.go

keep94 · 2022-04-15T15:08:04Z

It appears that unit tests in the exporter/prometheusremotewriteexporter are failing, which is a different exporter than what this PR is for. What can be done to resolve?

jpkrohling · 2022-04-18T14:36:29Z

Failed tests restarted.

keep94 · 2022-04-19T17:07:27Z

Even though the tests were restarted it looks like: github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter is still failing. I am not sure why. If possible, we would like to have this merged before the next big release because it fixes a severe backward compatibility bug with the config file.

keep94 · 2022-04-20T15:07:18Z

Hi. Thank you for restarting the tests. It looks like the build for this PR is now green. Are there any upcoming comments for this PR? As a gentle reminder we want to get this in a release as soon as possible because it fixes a major backward compatibility bug. Thank you.

keep94 · 2022-04-22T10:09:55Z

Hello. This code has been approved my my colleagues and the build is green. Is anything else needed for this code to be reviewed? This PR contains a fix for a backward compatibility bug in the configuration for this exporter. We would like it to be merged and released sooner than later. Thank you for your time. Hope to hear from you soon.

keep94 · 2022-04-25T23:01:01Z

@codeboten Hello. This PR, #9098, still needs to be reviewed by an owner. I notice that your name is on this PR to review it. Is there anything you need from us to facilitate review of this code? As a gentle reminder, this PR fixes a major bug related to backward compatibility issues in the config file; therefore, we would like this to be merged to the main branch as soon as possible. Thank you for your attention.

CHANGELOG.md

exporter/tanzuobservabilityexporter/config.go

TylerHelmuth · 2022-04-27T18:28:29Z

exporter/tanzuobservabilityexporter/config.go

+		}
+		tracesURL = &url.URL{}
+		*tracesURL = *metricsURL
+		tracesURL.Host = hostWithPort(metricsURL.Host, 30001)


I'm a little confused. If the user does not configure traces, this is adding traces configuration and using the supplied metrics endpoint right? I think the part I'm missing is why the traces settings need filled out in order for the exporter to work if it is only defined in a metrics pipeline? Or is the issue that users are adding the exporter to the trace pipeline but not supplying trace configuration?

Good point. I think the intention (please correct me @keep94) was to be able to separately specify metrics configuration and traces configuration and when only one is available, assume it's like the other but on a different (default) port.

Perhaps that's something tied to tanzuobservability but if not, I would suggest to consider approach similar to used in OTLP/HTTP. Either a single endpoint can be configured (in this case, this would be the hostname) or specifically traces_endpoint/metrics_endpoint/logs_endpoint.

Also, if endpoint for the signal was not defined and it's used in the pipeline it should just trigger an error

@pmm-sumo you are correct. If only the traces configuration is present, we assume the metrics configuration is like it but on a different port. Initially I wanted to throw an error if our tanzuobservability exporter is used in the metrics pipeline but no metrics configuration is defined, but I wasn't able to figure out how to do this in the code. I mean how can I tell if my exporter is used in a metrics pipeline from within the code?

I haven't verified this yet, but perhaps the createMetricsExporter function only gets called when our exporter is used in a metrics pipeline. Perhaps all I have to do is throw an error in createMetricsExporter if there is no metrics configuration. But, I just remembered that for tanzuobservability we have a unique requirement. Our traces exporter uses both the metrics and traces endpoint. Before we added the metrics exporter, we hardcoded the metrics endpoint to be the same as the traces endpoint except use port 2878. Now that we have both the metrics and traces exporter, we want the traces exporter to use the same metrics endpoint as the metrics exporter

Initially I wanted to throw an error if our tanzuobservability exporter is used in the metrics pipeline but no metrics configuration is defined, but I wasn't able to figure out how to do this in the code.

Unfortunately, Validate() can be only used in context of Config, without awareness of which pipeline it is. You might still throw an error when e.g. createTraces is called and not all config is available.

Another option is that you might prefer hostname and port numbers. The port numbers would have default values, so they would always point somewhere. This has the limitation of not supporting url's with paths.

Looking at the ideas, I would circle back to how otlphttp exporter does it. Perhaps you could have three fields:

hostname - if set, default port numbers are used and you can support both metrics and traces

metrics_endpoint - by default: "https://<hostname>:2878" but could be overwritten here if needed

traces_endpoint - be default: "https://<hostname>:30001" but could be overwritten here if needed

So a valid configuration would be:

hostname: myserver.example.com

or:

hostname: myserver.example.com metrics_endpoint: https://myotherserver.example.com:2878

or:

traces_endpoint: https://myserver1.example.com:30001 metrics_endpoint: https://myotherserver.example.com:2878

I think this idea a lot. I think it will simplify a lot of config.go, since you can be sure there is always an endpoint to use.

Hi @pmm-sumo and @TylerHelmuth: I'm a maintainer of this exporter (in addition to @keep94), and I had question. I'll let Travis consider your suggestions and reply on his own.

Your config changes are backwards-incompatible with our existing yaml. Is there an otel-collector procedure or policy for introducing breaking changes like this? Whether or not we go down that path in this PR, it'd be good to know how to go about breaking changes in the future. I'm pretty sure I've seen other exporters change or deprecate config options, so I do think it's possible. Thanks!

Hi @oppegard, I think this config change could be done in a non-breaking way. E.g. the new configuration fields could be added. When user uses the old one (essentially "endpoint"), a deprecation warning shows up in the logs.
After some time, the old configuration fields can be removed then.

Ultimately, I think it's your call since this is your exporter :)

Example of deprecated config:

opentelemetry-collector-contrib/processor/cumulativetodeltaprocessor/config.go

Line 35 in b9915f7

Metrics []string `mapstructure:"metrics"`

opentelemetry-collector-contrib/processor/cumulativetodeltaprocessor/processor.go

Line 111 in b9915f7

// Legacy support for deprecated Metrics config

Agree with @pmm-sumo, its your exporter so you can handle this however you want.

Thanks @pmm-sumo and @TylerHelmuth!

TylerHelmuth · 2022-04-29T00:18:38Z

Just did a quick review of this and so far I find this iteration easier to interpret. If I am reading things correctly, both a trace endpoint and metrics endpoint can be supplied, but they must have the same hostname. Trace endpoint must be supplied, but metrics endpoint is optional. If metrics endpoint is not supplied, a default port is used with the trace endpoint. If a metrics endpoint is supplied then the port on the supplied endpoint is used as the metrics port.

My only question right now is did you mean to remove your updates to the README.md? That was a beautiful README file with some great detail.

keep94 · 2022-04-29T00:19:50Z

Hello. Based on what was discussed, I now have brand new code. I will reach out again once my team has had a chance to approve the PR.

keep94 · 2022-04-29T00:25:25Z

@TylerHelmuth Thanks for pointing out the missing README.md, I will work on getting those changes back in this PR. As for your comment, both the traces and metrics endpoints are optional, neither have to be supplied, but if there is a traces pipeline, the traces endpoint must be supplied. If there is a metrics pipeline, the metrics endpoint must be supplied.

keep94 · 2022-05-01T02:19:05Z

Hello everyone. My tech lead approved these changes yesterday. You may resume reviewing this PR. Thank you.

pmm-sumo

I think it looks OK, need a rebasing though (we have released v0.50.0 in the meantime and this impacts changelog)

TylerHelmuth · 2022-05-02T15:53:52Z

@TylerHelmuth Thanks for pointing out the missing README.md, I will work on getting those changes back in this PR. As for your comment, both the traces and metrics endpoints are optional, neither have to be supplied, but if there is a traces pipeline, the traces endpoint must be supplied. If there is a metrics pipeline, the metrics endpoint must be supplied.

I either missed themetrics_exporter.go file or I took a look too soon. Looked over things again today and I continue to like this iteration more than the original.

keep94 · 2022-05-03T15:26:36Z

Hello. I just rebased this PR to resolve the conflicts in CHANGELOG.md

keep94 · 2022-05-03T23:08:01Z

It looks like CHANGELOG.md became out of data even since this morning when I rebased. Is there a way we can coordinate a time for when I should rebase and then somebody can merge immediately after I rebase?

keep94 · 2022-05-04T00:00:12Z

Just rebased to resolve conflicts with CHANGELOG.md

keep94 · 2022-05-04T15:39:54Z

Just now rebased to resolve conflicts in CHANGELOG.md. Please consider merging before CHANGELOG.md changes again. There are failing load tests, but I believe these failures are transient because the only changes made to this PR since it was approved was resolving conflicts in CHANGELOG.md

codeboten

@pmm-sumo please approve if your comments have been addressed 👍

pmm-sumo · 2022-05-04T16:02:13Z

@pmm-sumo please approve if your comments have been addressed 👍

Sure, will take a look later today

exporter/tanzuobservabilityexporter/config.go

exporter/tanzuobservabilityexporter/exporter.go

pmm-sumo

Some nits that are not blocking from my end

pmm-sumo · 2022-05-04T20:13:22Z

It looks like CHANGELOG.md became out of data even since this morning when I rebased. Is there a way we can coordinate a time for when I should rebase and then somebody can merge immediately after I rebase?

I think that such conflicts are actually trivial to resolve and I think usually this is done by one of the maintainers when merging the PR. So no need to worry about these :)

…open-telemetry#9098) * [exporter/tanzuobservability] Make metrics stanza optional in config. * Minor changes from pmm-sumo. * Add distribution port for delta histograms. Co-authored-by: Alex Boten <[email protected]>

keep94 requested review from a team and codeboten April 5, 2022 21:49

github-actions bot assigned dmitryax Apr 5, 2022

keep94 force-pushed the bug27996 branch 3 times, most recently from f5ed447 to f23c8c3 Compare April 9, 2022 00:29

keep94 mentioned this pull request Apr 11, 2022

[exporter/tanzuobservability] Add metrics to README. #7948

Closed

oppegard suggested changes Apr 11, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/exporter.go Outdated Show resolved Hide resolved

exporter/tanzuobservabilityexporter/metrics_exporter.go Outdated Show resolved Hide resolved

exporter/tanzuobservabilityexporter/metrics_exporter.go Outdated Show resolved Hide resolved

keep94 force-pushed the bug27996 branch from 69a7f05 to 6cb05fc Compare April 13, 2022 00:02

oppegard approved these changes Apr 13, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/exporter.go Outdated Show resolved Hide resolved

keep94 force-pushed the bug27996 branch from 6cb05fc to 69c4e41 Compare April 13, 2022 16:41

pmm-sumo reviewed Apr 27, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pmm-sumo reviewed Apr 27, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/config.go Outdated Show resolved Hide resolved

keep94 force-pushed the bug27996 branch from 8b168a7 to d439195 Compare April 27, 2022 17:40

TylerHelmuth reviewed Apr 27, 2022

View reviewed changes

keep94 force-pushed the bug27996 branch 2 times, most recently from 374fac7 to 860e98f Compare April 29, 2022 00:01

keep94 force-pushed the bug27996 branch from 860e98f to 547bd0b Compare April 29, 2022 00:51

oppegard approved these changes Apr 29, 2022

View reviewed changes

pmm-sumo reviewed May 2, 2022

View reviewed changes

TylerHelmuth approved these changes May 2, 2022

View reviewed changes

keep94 force-pushed the bug27996 branch from 547bd0b to 0da9c0b Compare May 3, 2022 15:24

keep94 force-pushed the bug27996 branch from 0da9c0b to c85b066 Compare May 3, 2022 23:59

[exporter/tanzuobservability] Make metrics stanza optional in config.

ca86be6

keep94 force-pushed the bug27996 branch from c85b066 to ca86be6 Compare May 4, 2022 15:15

codeboten reviewed May 4, 2022

View reviewed changes

pmm-sumo reviewed May 4, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/config.go Outdated Show resolved Hide resolved

pmm-sumo reviewed May 4, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/exporter.go Outdated Show resolved Hide resolved

pmm-sumo approved these changes May 4, 2022

View reviewed changes

Minor changes from pmm-sumo.

0326afb

keep94 and others added 2 commits May 5, 2022 14:39

Add distribution port for delta histograms.

8a00d6c

Merge branch 'main' into bug27996

b164797

codeboten added the ready to merge Code review completed; ready to merge by maintainers label May 5, 2022

codeboten approved these changes May 5, 2022

View reviewed changes

codeboten merged commit 78da6cd into open-telemetry:main May 5, 2022

ahayworth mentioned this pull request May 16, 2022

Update to otel-contrib-col v0.51.0 Shopify/opentelemetry-collector-contrib#796

Closed

oppegard mentioned this pull request Apr 12, 2023

REQUEST: New membership for oppegard open-telemetry/community#1421

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/tanzuobservability] Make metrics stanza optional in config. #9098

[exporter/tanzuobservability] Make metrics stanza optional in config. #9098

keep94 commented Apr 5, 2022

keep94 commented Apr 15, 2022

jpkrohling commented Apr 18, 2022

keep94 commented Apr 19, 2022

keep94 commented Apr 20, 2022

keep94 commented Apr 22, 2022

keep94 commented Apr 25, 2022

TylerHelmuth Apr 27, 2022 •

edited

Loading

pmm-sumo Apr 27, 2022

keep94 Apr 28, 2022

keep94 Apr 28, 2022

pmm-sumo Apr 28, 2022

TylerHelmuth Apr 28, 2022

oppegard Apr 28, 2022

pmm-sumo Apr 28, 2022

TylerHelmuth Apr 28, 2022

oppegard Apr 29, 2022

TylerHelmuth commented Apr 29, 2022

keep94 commented Apr 29, 2022

keep94 commented Apr 29, 2022

keep94 commented May 1, 2022

pmm-sumo left a comment •

edited

Loading

TylerHelmuth commented May 2, 2022

keep94 commented May 3, 2022

keep94 commented May 3, 2022

keep94 commented May 4, 2022

keep94 commented May 4, 2022

codeboten left a comment

pmm-sumo commented May 4, 2022

pmm-sumo left a comment

pmm-sumo commented May 4, 2022

[exporter/tanzuobservability] Make metrics stanza optional in config. #9098

[exporter/tanzuobservability] Make metrics stanza optional in config. #9098

Conversation

keep94 commented Apr 5, 2022

keep94 commented Apr 15, 2022

jpkrohling commented Apr 18, 2022

keep94 commented Apr 19, 2022

keep94 commented Apr 20, 2022

keep94 commented Apr 22, 2022

keep94 commented Apr 25, 2022

TylerHelmuth Apr 27, 2022 • edited Loading

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

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

TylerHelmuth commented Apr 29, 2022

keep94 commented Apr 29, 2022

keep94 commented Apr 29, 2022

keep94 commented May 1, 2022

pmm-sumo left a comment • edited Loading

Choose a reason for hiding this comment

TylerHelmuth commented May 2, 2022

keep94 commented May 3, 2022

keep94 commented May 3, 2022

keep94 commented May 4, 2022

keep94 commented May 4, 2022

codeboten left a comment

Choose a reason for hiding this comment

pmm-sumo commented May 4, 2022

pmm-sumo left a comment

Choose a reason for hiding this comment

pmm-sumo commented May 4, 2022

TylerHelmuth Apr 27, 2022 •

edited

Loading

pmm-sumo left a comment •

edited

Loading