Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement timeouts for both sending traces and telemetry. #518

Merged
merged 10 commits into from
Jul 18, 2024

Conversation

hoolioh
Copy link
Contributor

@hoolioh hoolioh commented Jul 3, 2024

What does this PR do?

This PR adds a new timeout field to the Endpoint struct in order to set a timeout for outgoing connections used for sending telemetry and traces.

Motivation

Prevent situations where holding too many connections could lead to memory pressure or resource exhaustion in the system by dropping them after a period of inactivity.

@pr-commenter
Copy link

pr-commenter bot commented Jul 3, 2024

Benchmarks

This comment was omitted because it was over 65536 characters.Please check the Gitlab Job logs to see its output.

@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch from e2ca938 to c93575a Compare July 3, 2024 11:16
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 93.42561% with 19 lines in your changes missing coverage. Please review.

Project coverage is 70.58%. Comparing base (f07597b) to head (702cceb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #518      +/-   ##
==========================================
+ Coverage   70.42%   70.58%   +0.16%     
==========================================
  Files         206      206              
  Lines       27964    28129     +165     
==========================================
+ Hits        19694    19856     +162     
- Misses       8270     8273       +3     
Components Coverage Δ
crashtracker 16.52% <86.66%> (-0.36%) ⬇️
datadog-alloc 98.73% <ø> (ø)
data-pipeline 50.00% <100.00%> (-1.16%) ⬇️
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.43% <85.36%> (+0.01%) ⬆️
ddcommon-ffi 75.29% <76.19%> (-0.02%) ⬇️
ddtelemetry 58.95% <89.47%> (-0.08%) ⬇️
ipc 84.13% <ø> (ø)
profiling 78.78% <93.54%> (+0.09%) ⬆️
profiling-ffi 58.64% <95.83%> (+0.37%) ⬆️
serverless 0.00% <ø> (ø)
sidecar 35.42% <88.23%> (-0.28%) ⬇️
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 71.14% <100.00%> (+0.10%) ⬆️
trace-normalization 98.24% <ø> (ø)
trace-obfuscation 95.73% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 91.03% <98.62%> (+0.28%) ⬆️

@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch from ca17e83 to 85dc749 Compare July 3, 2024 13:51
@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch from 1ed553b to 0513e2b Compare July 3, 2024 14:29
@bwoebi
Copy link
Contributor

bwoebi commented Jul 3, 2024

Slightly surprised that we opt modifying the Endpoint struct, but it probably makes sense :-)

Can you please also add a FFI API to set the timeout on the Endpoint?

@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch from 0513e2b to 85dc749 Compare July 3, 2024 16:07
@github-actions github-actions bot removed the ci-build label Jul 3, 2024
@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch 5 times, most recently from 2efdfd2 to 6f93cd0 Compare July 4, 2024 11:46
ddcommon/src/lib.rs Outdated Show resolved Hide resolved
@hoolioh hoolioh marked this pull request as ready for review July 4, 2024 12:58
@hoolioh hoolioh requested review from a team as code owners July 4, 2024 12:58
@hoolioh hoolioh requested a review from omerli July 4, 2024 12:58
sidecar/src/dogstatsd.rs Outdated Show resolved Hide resolved
sidecar/src/dogstatsd.rs Outdated Show resolved Hide resolved
@danielsn danielsn self-requested a review July 9, 2024 19:48
@danielsn danielsn dismissed their stale review July 9, 2024 19:49

Required changes were made

@hoolioh hoolioh requested a review from a team as a code owner July 10, 2024 10:16
@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch 4 times, most recently from 1a38259 to 18f54cb Compare July 10, 2024 10:52
Copy link
Contributor

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks!

@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch 3 times, most recently from 1265a2f to 949da71 Compare July 12, 2024 11:43
Copy link
Contributor

@danielsn danielsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, modulo some small comments.

crashtracker/src/telemetry.rs Outdated Show resolved Hide resolved
crashtracker/src/telemetry.rs Outdated Show resolved Hide resolved
profiling-ffi/src/exporter.rs Outdated Show resolved Hide resolved
bin_tests/src/bin/crashtracker_bin_test.rs Outdated Show resolved Hide resolved
crashtracker/src/api.rs Outdated Show resolved Hide resolved
data-pipeline/src/trace_exporter.rs Show resolved Hide resolved
examples/ffi/exporter.cpp Outdated Show resolved Hide resolved
profiling-ffi/src/exporter.rs Show resolved Hide resolved
profiling/tests/form.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data/mod.rs Outdated Show resolved Hide resolved
@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch 2 times, most recently from 0f35bf4 to 253fcb0 Compare July 17, 2024 16:23
@hoolioh hoolioh force-pushed the julio/APMSP-1235-implement-timeouts branch from 253fcb0 to 702cceb Compare July 18, 2024 08:01
@hoolioh hoolioh merged commit d351d66 into main Jul 18, 2024
32 checks passed
@hoolioh hoolioh deleted the julio/APMSP-1235-implement-timeouts branch July 18, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants