Skip to content

Change extension detection mechanism #131

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

Merged
merged 8 commits into from
Nov 30, 2020

Conversation

DarcyRaynerDD
Copy link
Contributor

What does this PR do?

Screen Shot 2020-11-24 at 11 15 29 AM

Changes the extension detection mechanism to check if the extension binary is present locally. Also, moves calls to extension outside of root trace span.

Motivation

Currently, checking for the datadog extension is done by attempting to open a tcp connection to the extension's endpoints on localhost. When the extension is present this request will fail. This has the side effect of generating an error span in our trace on cold starts. Instead, we swap to checking for access to the extension file first, before making the tcp connection.

The second change we make is to call extension methods, (including flush), outside of the main trace. This reduces the noise in the trace, and means the trace can now be flushed on the same invocation that created it.

Testing Guidelines

Updated integration tests, and added unit tests.

Additional Notes

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (ask a Datadog member to run the tests)

@DarcyRaynerDD DarcyRaynerDD requested a review from a team as a code owner November 24, 2020 17:46
@codecov-io
Copy link

Codecov Report

Merging #131 (f7c3082) into master (3693796) will decrease coverage by 0.04%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage   87.68%   87.64%   -0.05%     
==========================================
  Files          29       29              
  Lines         958      971      +13     
  Branches      169      169              
==========================================
+ Hits          840      851      +11     
- Misses         80       82       +2     
  Partials       38       38              
Impacted Files Coverage Δ
src/index.ts 83.33% <88.23%> (-1.45%) ⬇️
src/metrics/extension.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3693796...f7c3082. Read the comment docs.

Copy link
Contributor

@Czechh Czechh left a comment

Choose a reason for hiding this comment

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

👏 - love the refactor

@DarcyRaynerDD DarcyRaynerDD merged commit e0116d3 into master Nov 30, 2020
@DarcyRaynerDD DarcyRaynerDD deleted the darcy.rayner/change-extension-detection branch November 30, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants