[exporter/datadog] Add `api.fail_on_invalid_key` to fail fast if API Key is invalid #9426

keisku · 2022-04-25T13:57:46Z

Description:

Add api.fail_on_invalid_key to fail fast if API Key is invalid.

Link to tracking Issue:

Testing:

fail_on_invalid_key: false

1 ) Set the invalid API key and fail_on_invalid_key: false.
2 ) make run
3 ) Confirming error message
This behavior is same as before.

GO111MODULE=on go run --race ./cmd/otelcontribcol/... --config local/config.yaml
2022/04/28 06:42:12 proto: duplicate proto type registered: jaeger.api_v2.PostSpansRequest
2022/04/28 06:42:12 proto: duplicate proto type registered: jaeger.api_v2.PostSpansResponse
2022-04-28T06:42:12.532+0900    info    datadogexporter/factory.go:195  sanitizing Datadog traces exporter configuration        {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:12.532+0900    info    utils/api.go:37 Validating API key.     {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:13.262+0900    warn    utils/api.go:47 API Key validation failed       {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:13.263+0900    info    datadogexporter/factory.go:135  sanitizing Datadog metrics exporter configuration       {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:13.263+0900    info    utils/api.go:37 Validating API key.     {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:13.966+0900    warn    utils/api.go:47 API Key validation failed       {"kind": "exporter", "name": "datadog"}
2022-04-28T06:42:13.967+0900    info    builder/exporters_builder.go:255        Exporter was built.     {"kind": "exporter", "name": "datadog"}

fail_on_invalid_key: true

1 ) Set the invalid API key and fail_on_invalid_key: true.
2 ) make run
3 ) Confirming error message

GO111MODULE=on go run --race ./cmd/otelcontribcol/... --config local/config.yaml
2022/04/28 06:40:36 proto: duplicate proto type registered: jaeger.api_v2.PostSpansRequest
2022/04/28 06:40:36 proto: duplicate proto type registered: jaeger.api_v2.PostSpansResponse
2022-04-28T06:40:36.322+0900    info    datadogexporter/factory.go:195  sanitizing Datadog traces exporter configuration        {"kind": "exporter", "name": "datadog"}
2022-04-28T06:40:36.322+0900    info    utils/api.go:37 Validating API key.     {"kind": "exporter", "name": "datadog"}
2022-04-28T06:40:37.111+0900    warn    utils/api.go:47 API Key validation failed       {"kind": "exporter", "name": "datadog"}
Error: cannot build exporters: error creating datadog exporter: API Key validation failed
2022/04/28 06:40:37 collector server run finished with error: cannot build exporters: error creating datadog exporter: API Key validation failed
exit status 1
make: *** [run] Error 1

Documentation:

dashpole

I don't know if a config option is appropriate here. If configuration is invalid, the component should always fail IMO. If an exporter cannot be started as configured, it should return the error in its newMetricsExporter function, for example.

mx-psi · 2022-04-27T16:48:37Z

I don't know if a config option is appropriate here. If configuration is invalid, the component should always fail IMO. If an exporter cannot be started as configured, it should return the error in its newMetricsExporter function, for example.

I agree in principle, but there is some internal hesitancy to failing to start by default because of a feature that may be added to Datadog in the future related to API keys, so I think we want it behind a flag, even if I agree this looks odd from the outside

exporter/datadogexporter/internal/utils/api.go

exporter/datadogexporter/example/config.yaml

exporter/datadogexporter/config/config.go

mx-psi

Thanks for doing this! My main comment is that instead of panicking with logger.Fatal, we should return an error. I left some other comments inline

exporter/datadogexporter/internal/utils/api.go

mx-psi

Looks great, thanks for addressing the comments! 🙇 Just two tiny nits, after those are addressed I will wait for David to ACK my comment and then mark as ready to merge

exporter/datadogexporter/factory_test.go

Lint is failing

mx-psi · 2022-04-28T12:03:33Z

It looks like you also need to address some lint errors: https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/6209762394?check_suite_focus=true

exporter/datadogexporter/factory.go

codeboten · 2022-05-04T23:43:22Z

Closing and re-opening PR to trigger the gotidy CI check

mx-psi · 2022-05-05T15:46:26Z

Looks like it passed now

gbbr · 2022-05-06T06:23:17Z

exporter/datadogexporter/factory.go

@@ -198,6 +198,7 @@ func (f *factory) createTracesExporter(
 	}

 	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()


Actually this cancels the exporter prematurely after the function exits. The cancel is only meant to be called on shutdown (it is part of shutdown, so technically it does get called on all paths).

(it is part of shutdown, so technically it does get called on all paths).

the lint was failing because it was not called when the traces exporter fails to get created. I opened #9797 to fix this, thanks for spotting that!

…Key is invalid (open-telemetry#9426) * add new `fail_on_invalid_key` option * improve the description of fail_on_invalid_key * avoid failing when a transient network error * remove unnecessary comment * utils.ValidateAPI() returns error * add tests * not returning error when api failure * fix test names * fix lint * fix go vet: prevent possible context leak Co-authored-by: Alex Boten <aboten@lightstep.com>

project-bot bot added this to In progress in Collector Apr 25, 2022

keisku marked this pull request as ready for review April 25, 2022 14:42

keisku requested review from a team and mx-psi as code owners April 25, 2022 14:42

github-actions bot assigned dashpole Apr 25, 2022

keisku changed the title ~~[exporter/datadog] validate API Key by using Datadog API~~ [exporter/datadog] Fail fast if API Key is invalid Apr 25, 2022

keisku marked this pull request as draft April 25, 2022 15:04

keisku force-pushed the keisku/validate-api-key branch 3 times, most recently from f95233b to 4e2d4cc Compare April 25, 2022 15:29

keisku marked this pull request as ready for review April 25, 2022 15:30

github-actions bot assigned Aneurysm9 Apr 25, 2022

keisku changed the title ~~[exporter/datadog] Fail fast if API Key is invalid~~ [exporter/datadog] Add api.fail_on_invalid_key to fail fast if API Key is invalid. Apr 25, 2022

keisku changed the title ~~[exporter/datadog] Add api.fail_on_invalid_key to fail fast if API Key is invalid.~~ [exporter/datadog] Add api.fail_on_invalid_key to fail fast if API Key is invalid Apr 25, 2022

add new fail_on_invalid_key option

14f929f

keisku force-pushed the keisku/validate-api-key branch from 4e2d4cc to 14f929f Compare April 25, 2022 15:33

dashpole reviewed Apr 27, 2022

View reviewed changes

mx-psi requested changes Apr 27, 2022

View reviewed changes

Collector automation moved this from In progress to Review in progress Apr 27, 2022

mx-psi requested changes Apr 27, 2022

View reviewed changes

keisku added 4 commits April 28, 2022 06:08

improve the description of fail_on_invalid_key

8150b63

avoid failing when a transient network error

f375265

remove unnecessary comment

d8e4a89

utils.ValidateAPI() returns error

624b4a1

keisku requested a review from mx-psi April 27, 2022 22:54

add tests

e3c8cf2

keisku force-pushed the keisku/validate-api-key branch from 3f1f106 to e3c8cf2 Compare April 27, 2022 23:13

mx-psi reviewed Apr 28, 2022

View reviewed changes

exporter/datadogexporter/internal/utils/api.go Outdated Show resolved Hide resolved

not returning error when api failure

006a462

keisku requested a review from mx-psi April 28, 2022 11:09

mx-psi linked an issue Apr 28, 2022 that may be closed by this pull request

[exporter/datadog] datadog exporter should fail fast if api key validation fail #7402

Closed

mx-psi previously approved these changes Apr 28, 2022

View reviewed changes

exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved

exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved

Collector automation moved this from Review in progress to Reviewer approved Apr 28, 2022

Collector automation moved this from Reviewer approved to Review in progress Apr 28, 2022

keisku added 3 commits April 28, 2022 22:45

fix test names

80edbdb

fix lint

368e67d

fix go vet: prevent possible context leak

b6ac7f9

keisku commented Apr 28, 2022

View reviewed changes

exporter/datadogexporter/factory.go Show resolved Hide resolved

keisku requested a review from mx-psi April 28, 2022 13:57

mx-psi approved these changes Apr 28, 2022

View reviewed changes

Collector automation moved this from Review in progress to Reviewer approved Apr 28, 2022

Merge branch 'main' into keisku/validate-api-key

2a6d697

codeboten closed this May 4, 2022

codeboten reopened this May 4, 2022

Collector automation moved this from Reviewer approved to In progress May 4, 2022

codeboten approved these changes May 4, 2022

View reviewed changes

Collector automation moved this from In progress to Reviewer approved May 4, 2022

codeboten merged commit 1d6b6fb into open-telemetry:main May 5, 2022

gbbr reviewed May 6, 2022

View reviewed changes

mx-psi mentioned this pull request May 9, 2022

[exporter/datadog] Fix canceling on createTracesExporter #9797

Merged

ahayworth mentioned this pull request May 16, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/datadog] Add `api.fail_on_invalid_key` to fail fast if API Key is invalid #9426

[exporter/datadog] Add `api.fail_on_invalid_key` to fail fast if API Key is invalid #9426

keisku commented Apr 25, 2022 •

edited

dashpole left a comment

mx-psi commented Apr 27, 2022

mx-psi left a comment

mx-psi left a comment

mx-psi commented Apr 28, 2022

codeboten commented May 4, 2022

mx-psi commented May 5, 2022

gbbr May 6, 2022

mx-psi May 9, 2022

[exporter/datadog] Add api.fail_on_invalid_key to fail fast if API Key is invalid #9426

[exporter/datadog] Add api.fail_on_invalid_key to fail fast if API Key is invalid #9426

Conversation

keisku commented Apr 25, 2022 • edited

dashpole left a comment

Choose a reason for hiding this comment

mx-psi commented Apr 27, 2022

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi commented Apr 28, 2022

codeboten commented May 4, 2022

mx-psi commented May 5, 2022

gbbr May 6, 2022

Choose a reason for hiding this comment

mx-psi May 9, 2022

Choose a reason for hiding this comment

[exporter/datadog] Add `api.fail_on_invalid_key` to fail fast if API Key is invalid #9426

[exporter/datadog] Add `api.fail_on_invalid_key` to fail fast if API Key is invalid #9426

keisku commented Apr 25, 2022 •

edited