Skip to content

Conversation

@mawais54013
Copy link
Contributor

@mawais54013 mawais54013 commented Aug 3, 2022

Description

Enabling payload partial flush by default

Resolves #1632

Checklist

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • PR cannot be broken up into smaller PRs.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added for fixes and features, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.
  • Add to milestone.

@mawais54013 mawais54013 requested a review from a team as a code owner August 3, 2022 17:28
@mawais54013 mawais54013 added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 3, 2022
@mawais54013 mawais54013 changed the title chore(ci):enabling partial flushing bu default chore(ci):enabling partial flushing by default Aug 3, 2022
@mawais54013 mawais54013 force-pushed the mawais54013/enabling-partialflushing branch from 417f0d7 to 53394d8 Compare August 3, 2022 19:15
@ZStriker19 ZStriker19 changed the title chore(ci):enabling partial flushing by default chore(ci): enabling partial flushing by default Aug 3, 2022
@mawais54013 mawais54013 force-pushed the mawais54013/enabling-partialflushing branch from fbb52bd to 6f99023 Compare August 3, 2022 20:27
@mawais54013
Copy link
Contributor Author

Not sure why the integration tests are failing as in some cases, they do enable the partial flush but even setting partial_flush_enabled=False in functions like test_metrics and test_single_trace_too_large, they still seem to be failing.

Likewise, asyncio and ddtracerun do not use partial flush in their code so not sure if it's expected that they should be failing.

@mawais54013 mawais54013 force-pushed the mawais54013/enabling-partialflushing branch from f914c1b to b13450d Compare August 4, 2022 19:55
@mawais54013 mawais54013 force-pushed the mawais54013/enabling-partialflushing branch from b13450d to 3dfa76e Compare August 4, 2022 20:00
Copy link
Contributor Author

@mawais54013 mawais54013 left a comment

Choose a reason for hiding this comment

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

It seems like in some cases depending on the python version, the calls length might be longer than expected. Is the log.warning necessary in this case or is there a better and dynamic way of handling this behavior

Co-authored-by: Munir Abdinur <munir.abdinur@datadoghq.com>
ZStriker19
ZStriker19 previously approved these changes Aug 5, 2022
Copy link
Collaborator

@ZStriker19 ZStriker19 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, should we make docs changes or do we just never document this?

mabdinur
mabdinur previously approved these changes Aug 15, 2022
Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

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

LGTM 🥳

ZStriker19
ZStriker19 previously approved these changes Aug 15, 2022
Copy link
Collaborator

@ZStriker19 ZStriker19 left a comment

Choose a reason for hiding this comment

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

small nit but feel free to ignore if you like your wording more.

Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com>
@mawais54013 mawais54013 dismissed stale reviews from ZStriker19 and mabdinur via c317bcf August 15, 2022 21:30
Copy link
Contributor

@Yun-Kim Yun-Kim left a comment

Choose a reason for hiding this comment

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

LGTM other than the one test case to add 😄

Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

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

LGTM 🥳

@mabdinur mabdinur requested review from Yun-Kim and ZStriker19 August 19, 2022 21:12
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

Merging #4041 (64696bd) into 1.x (defa101) will decrease coverage by 0.04%.
The diff coverage is 7.50%.

@@            Coverage Diff             @@
##              1.x    #4041      +/-   ##
==========================================
- Coverage   78.27%   78.23%   -0.05%     
==========================================
  Files         726      726              
  Lines       58268    58305      +37     
==========================================
+ Hits        45612    45614       +2     
- Misses      12656    12691      +35     
Impacted Files Coverage Δ
tests/commands/test_runner.py 0.00% <ø> (ø)
tests/integration/test_integration.py 0.00% <0.00%> (ø)
ddtrace/tracer.py 88.85% <100.00%> (ø)
tests/contrib/asyncio/test_tracer_safety.py 100.00% <100.00%> (ø)
ddtrace/contrib/cassandra/session.py 87.84% <0.00%> (+1.10%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Kyle-Verhoog
Copy link
Member

Looks like the appsec test suite is failing on the last couple CI runs

@majorgreys
Copy link
Contributor

Blocking for now until we have a handle on end-to-end testing of this feature.

@mergify mergify bot merged commit 2d4c038 into 1.x Sep 7, 2022
@mergify mergify bot deleted the mawais54013/enabling-partialflushing branch September 7, 2022 17:21
@github-actions github-actions bot added this to the v1.5.0 milestone Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/no-changelog A changelog entry is not required for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak while handling large trace from long-running celery tasks

10 participants