feat(otlp-exporter): add timeout env var #2738

svetlanabrennan · 2022-01-25T00:19:45Z

Signed-off-by: Svetlana Brennan [email protected]

Which problem is this PR solving?

Partially Fixes #2706 (issue)

This is just the first PR for issue #2706 that adds OTEL_EXPORTER_OTLP_TIMEOUT & OTEL_EXPORTER_OTLP_TRACES_TIMEOUT as an environment variable for the otlp trace exporters. I wanted to split this issue up in separate prs so it's easier to review and I need this timeout variable for another pr regarding retry logic I'm working on.

Short description of the changes

Added OTEL_EXPORTER_OTLP_TIMEOUT as default environment variable
Added OTEL_EXPORTER_OTLP_TRACES_TIMEOUT as default environment variable
Added timeoutMillis property to the OTLPExporterBase class
Added timeout logic to http request (sendWithHttp utility function)
Added timeout logic to xhr request (sendWithXhr utility function)
Added timeout logic to grpc request (send utility function)

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit tests
Integration tests

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

Signed-off-by: Svetlana Brennan <[email protected]>

codecov · 2022-01-25T00:22:06Z

Codecov Report

Merging #2738 (da4e4c8) into main (ac578e9) will decrease coverage by 0.27%.
The diff coverage is 40.00%.

❗ Current head da4e4c8 differs from pull request most recent head 6ee89b0. Consider uploading reports for the commit 6ee89b0 to get more accurate results

@@            Coverage Diff             @@
##             main    #2738      +/-   ##
==========================================
- Coverage   92.78%   92.51%   -0.28%     
==========================================
  Files         183      183              
  Lines        5921     5958      +37     
  Branches     1257     1266       +9     
==========================================
+ Hits         5494     5512      +18     
- Misses        427      446      +19

Impacted Files	Coverage Δ
...kages/otlp-exporter-base/src/platform/node/util.ts	`21.33% <0.00%> (-6.26%)`	⬇️
...erimental/packages/otlp-exporter-base/src/types.ts	`16.66% <ø> (ø)`
...ntal/packages/otlp-grpc-exporter-base/src/types.ts	`100.00% <ø> (ø)`
...ental/packages/otlp-grpc-exporter-base/src/util.ts	`67.18% <0.00%> (-1.07%)`	⬇️
...ckages/opentelemetry-core/src/utils/environment.ts	`96.00% <ø> (ø)`
...perimental/packages/otlp-exporter-base/src/util.ts	`89.65% <94.11%> (+6.32%)`	⬆️
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts	`95.23% <100.00%> (+0.23%)`	⬆️

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-01-25T23:21:00Z

I haven't updated documentation yet. I would like some feedback on this pr before I update documentation please.

svetlanabrennan · 2022-01-26T18:45:27Z

Does OTEL_EXPORTER_OTLP_TRACES_TIMEOUT take precedence over OTEL_EXPORTER_OTLP_TIMEOUT?

Signed-off-by: Svetlana Brennan <[email protected]>

…ntelemetry-js into add-exporter-timeout Merge main into add-exporter-timeout

vmarchaud · 2022-01-26T19:39:36Z

oes OTEL_EXPORTER_OTLP_TRACES_TIMEOUT take precedence over OTEL_EXPORTER_OTLP_TIMEOUT

Yes it should, signal specific configs take priority over generic ones

svetlanabrennan · 2022-01-26T19:42:24Z

Got it. I just implemented that logic for OTEL_EXPORTER_OTLP_TRACES_TIMEOUT to take precedence over OTEL_EXPORTER_OTLP_TIMEOUT and refactored some code.

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-01-28T17:16:37Z

Added timeout config information to each otlp trace exporter readme.

This pr is ready for review/merge from my end.

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts

Signed-off-by: Svetlana Brennan <[email protected]>

packages/opentelemetry-core/test/utils/environment.test.ts

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts

packages/exporter-trace-otlp-grpc/README.md

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts

Signed-off-by: Svetlana Brennan <[email protected]>

…ntelemetry-js into add-exporter-timeout

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-04-26T18:57:51Z

Lint test is passing now. The codecov tests are failing but they were passing previously. The codecov path test is saying that the sendWithHttp function isn't being test but it is being tested - the tests are just located in another package.

Should I move these tests into the otlp-exporter-base package?

svetlanabrennan · 2022-04-27T17:50:28Z

Tests failing after main merge. I'm seeing this test failing in the 'main' branch as well (when I run the tests locally).

Flarna · 2022-04-27T18:32:38Z

@dyladan moved the latest label of @opentelemetry/api from verion 1.04 to 1.1.0. This could be the cause here in case several API versions are installed.

Flarna · 2022-04-27T19:54:03Z

local tests on main are green for me. Seems like CI specific failures. Maybe some caching?

svetlanabrennan · 2022-04-28T16:48:26Z

It must have been the cache. The local tests are passing now both in main and in this pr branch.

svetlanabrennan · 2022-04-28T16:50:05Z

@dyladan moved the latest label of @opentelemetry/api from verion 1.04 to 1.1.0. This could be the cause here in case several API versions are installed.

Was this a codecov config change? I assume so because these tests are all passing when I run it locally.

dyladan · 2022-04-28T17:29:31Z

Codecov would not affect the tests anyway but this is probably because the 1.1 api is now latest. There is most likely a version mismatch somewhere. I'm on my phone and ooo right now but may be able to look later if I have time

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-05-05T18:59:47Z

Looks like 1 browser test is failing after merging main into this branch. Not sure why. All tests passing when I run it locally.

Flarna · 2022-05-05T19:49:17Z

re-triggered the test and it's green now. Seems to be a test issue

svetlanabrennan · 2022-05-05T21:12:45Z

I think there's also a test issue with the codecov for "codecov/patch" because it's indicating that some lines are not covered by tests but they are tested.

For example, it says line 54 isn't tested in this file but there is a test for it here.

legendecas

LGTM % nits

experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts

dyladan

LGTM % legendecas' nit

experimental/CHANGELOG.md

… tests Signed-off-by: Svetlana Brennan <[email protected]>

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-05-09T21:37:48Z

One test failing in an unrelated package (didn't work on that package in this pr).

Could this be a test issue again like @Flarna mentioned earlier for another time an unrelated test was failing?

…ts with settimeout Signed-off-by: Svetlana Brennan <[email protected]>

dyladan · 2022-05-10T17:20:39Z

Failure seems to be in the OTLPTraceExporter which is touched by this PR

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-05-10T21:04:52Z

Failure seems to be in the OTLPTraceExporter which is touched by this PR

The real http request tests were failing randomly. I think it was a timing issue. So I moved those tests to their own test suite and it's passing now. Also ran it locally a handful of times and it's all green as well.

dyladan

Overall LGTM was just a bit surprised to see the behavior split by major node version

dyladan · 2022-05-11T12:57:07Z

experimental/CHANGELOG.md

@@ -60,6 +60,7 @@ All notable changes to experimental packages in this project will be documented
 * feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746 @dyladan
 * feat(sdk-metrics-base): shutdown and forceflush on MeterProvider #2890 @legendecas
 * feat(sdk-metrics-base): return the same meter for identical input to getMeter #2901 @legendecas
+* feat(otlp-exporter): add [OTEL_EXPORTER_OTLP_TIMEOUT](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#configuration-options) env var to otlp exporters #2738 @svetlanabrennan


experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts

experimental/packages/otlp-exporter-base/src/platform/node/util.ts

Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): add timeout env var

4e81f83

Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): add timeout env var test

11ea770

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan marked this pull request as ready for review January 25, 2022 23:19

svetlanabrennan requested a review from a team January 25, 2022 23:19

Merge branch 'main' into add-exporter-timeout

a9a526f

svetlanabrennan and others added 3 commits January 26, 2022 13:32

Merge branch 'main' into add-exporter-timeout

4f659fb

feat(otlp-exporter): add trace_timeout env var

2bc61e4

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…

48a25d7

…ntelemetry-js into add-exporter-timeout Merge main into add-exporter-timeout

vmarchaud reviewed Jan 26, 2022

View reviewed changes

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts Outdated Show resolved Hide resolved

vmarchaud reviewed Jan 26, 2022

View reviewed changes

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts Outdated Show resolved Hide resolved

vmarchaud reviewed Jan 26, 2022

View reviewed changes

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts Outdated Show resolved Hide resolved

svetlanabrennan added 3 commits January 26, 2022 17:17

feat(otlp-exporter): add how timeout is selected

da3211f

Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): add negative value check to env vars

0c1011d

Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): add negative value check to env vars

ed4079f

Signed-off-by: Svetlana Brennan <[email protected]>

vmarchaud approved these changes Jan 28, 2022

View reviewed changes

feat(otlp-exporter): add timeout config information to readme

fc486a1

Signed-off-by: Svetlana Brennan <[email protected]>

dyladan reviewed Jan 28, 2022

View reviewed changes

packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts Outdated Show resolved Hide resolved

feat(otlp-exporter): add const var for default timeout value

7292244

Signed-off-by: Svetlana Brennan <[email protected]>

vmarchaud added the enhancement New feature or request label Jan 29, 2022

svetlanabrennan added 2 commits January 31, 2022 09:43

Merge branch 'main' into add-exporter-timeout

ca98326

Merge branch 'main' into add-exporter-timeout

5e7d2f5

blumamir reviewed Feb 1, 2022

View reviewed changes

svetlanabrennan added 2 commits February 1, 2022 17:26

feat(otlp-exporter): refactor code based on feedback

e8dc21b

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…

6c3a42d

…ntelemetry-js into add-exporter-timeout

feat(otlp-exporter): fix lint

e81f9dd

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-exporter-timeout

ae49cd3

vmarchaud and others added 3 commits April 30, 2022 13:53

Merge branch 'main' into add-exporter-timeout

828993e

Merge branch 'main' into add-exporter-timeout

dc647fd

feat(otlp-exporter): fix accidentally removed code after main merge

fbaa457

Signed-off-by: Svetlana Brennan <[email protected]>

legendecas approved these changes May 9, 2022

View reviewed changes

experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts Outdated Show resolved Hide resolved

experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts Outdated Show resolved Hide resolved

dyladan approved these changes May 9, 2022

View reviewed changes

experimental/CHANGELOG.md Outdated Show resolved Hide resolved

svetlanabrennan and others added 3 commits May 9, 2022 15:42

feat(otlp-exporter): replace nextTick with queueMicroTask for browser…

14ba7d3

… tests Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): add more details to changelog item

1ebc461

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-exporter-timeout

27b2b86

svetlanabrennan and others added 2 commits May 10, 2022 10:05

Merge branch 'main' into add-exporter-timeout

b186cbf

feat(otlp-exporter): fix server timing issue in real http request tes…

511999a

…ts with settimeout Signed-off-by: Svetlana Brennan <[email protected]>

feat(otlp-exporter): fix real http request tests

b553759

Signed-off-by: Svetlana Brennan <[email protected]>

dyladan reviewed May 11, 2022

View reviewed changes

svetlanabrennan and others added 2 commits May 11, 2022 11:26

feat(otlp-exporter): add comment to two ways of destroying request

5da6ce2

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-exporter-timeout

6ee89b0

dyladan merged commit 479321c into open-telemetry:main May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(otlp-exporter): add timeout env var #2738

feat(otlp-exporter): add timeout env var #2738

svetlanabrennan commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

svetlanabrennan commented Jan 25, 2022

svetlanabrennan commented Jan 26, 2022

vmarchaud commented Jan 26, 2022

svetlanabrennan commented Jan 26, 2022

svetlanabrennan commented Jan 28, 2022

svetlanabrennan commented Apr 26, 2022

svetlanabrennan commented Apr 27, 2022

Flarna commented Apr 27, 2022 •

edited

Loading

Flarna commented Apr 27, 2022

svetlanabrennan commented Apr 28, 2022

svetlanabrennan commented Apr 28, 2022

dyladan commented Apr 28, 2022

svetlanabrennan commented May 5, 2022

Flarna commented May 5, 2022

svetlanabrennan commented May 5, 2022

legendecas left a comment

dyladan left a comment

svetlanabrennan commented May 9, 2022

dyladan commented May 10, 2022

svetlanabrennan commented May 10, 2022

dyladan left a comment

dyladan May 11, 2022

feat(otlp-exporter): add timeout env var #2738

feat(otlp-exporter): add timeout env var #2738

Conversation

svetlanabrennan commented Jan 25, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jan 25, 2022 • edited Loading

Codecov Report

svetlanabrennan commented Jan 25, 2022

svetlanabrennan commented Jan 26, 2022

vmarchaud commented Jan 26, 2022

svetlanabrennan commented Jan 26, 2022

svetlanabrennan commented Jan 28, 2022

svetlanabrennan commented Apr 26, 2022

svetlanabrennan commented Apr 27, 2022

Flarna commented Apr 27, 2022 • edited Loading

Flarna commented Apr 27, 2022

svetlanabrennan commented Apr 28, 2022

svetlanabrennan commented Apr 28, 2022

dyladan commented Apr 28, 2022

svetlanabrennan commented May 5, 2022

Flarna commented May 5, 2022

svetlanabrennan commented May 5, 2022

legendecas left a comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

svetlanabrennan commented May 9, 2022

dyladan commented May 10, 2022

svetlanabrennan commented May 10, 2022

dyladan left a comment

Choose a reason for hiding this comment

dyladan May 11, 2022

Choose a reason for hiding this comment

svetlanabrennan commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

Flarna commented Apr 27, 2022 •

edited

Loading