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

Protect the connection string tags extractor from an invalid connection string #5956

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

tonyredondo
Copy link
Member

Summary of changes

This PR fixes an scenario when the connection string could not be parsed by avoiding the exception.

Reason for change

Seen in Error tracking:

image

Implementation details

Wrap the method in a try catch block, and return default on that invalid connection string.
The caller method already returns default in other cases.

Test coverage

This is an small fix, I didn't write a test case for this... yet.

Other details

@tonyredondo tonyredondo requested a review from a team as a code owner August 28, 2024 10:54
@tonyredondo tonyredondo self-assigned this Aug 28, 2024
@github-actions github-actions bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Aug 28, 2024
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Aug 28, 2024

Datadog Report

Branch report: tony/fix-connectionstring-cache-issue
Commit report: 76f8db9
Test service: dd-trace-dotnet

✅ 0 Failed, 300951 Passed, 1584 Skipped, 11h 51m 59.76s Total Time

@andrewlock
Copy link
Member

andrewlock commented Aug 28, 2024

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

tracer/src/Datadog.Trace/Util/DbCommandCache.cs Outdated Show resolved Hide resolved
@tonyredondo tonyredondo merged commit 5efa35e into master Aug 28, 2024
66 of 70 checks passed
@tonyredondo tonyredondo deleted the tony/fix-connectionstring-cache-issue branch August 28, 2024 13:50
@github-actions github-actions bot added this to the vNext-v3 milestone Aug 28, 2024
tonyredondo added a commit that referenced this pull request Aug 28, 2024
…on string (#5956)

## Summary of changes

This PR fixes an scenario when the connection string could not be parsed
by avoiding the exception.

## Reason for change

Seen in Error tracking:

<img width="1127" alt="image"
src="https://github.com/user-attachments/assets/436de8fe-b6b8-43d7-a894-5554bb4e04eb">

## Implementation details

Wrap the method in a try catch block, and return default on that invalid
connection string.
The caller method already returns `default` in other cases.

## Test coverage

This is an small fix, I didn't write a test case for this... yet.

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->

---------

Co-authored-by: Andrew Lock <[email protected]>
tonyredondo added a commit that referenced this pull request Aug 29, 2024
…on string (#5956 -> v2) (#5960)

## Summary of changes

This PR fixes an scenario when the connection string could not be parsed
by avoiding the exception.

## Reason for change

Seen in Error tracking:

<img width="1127" alt="image"

src="https://github.com/user-attachments/assets/436de8fe-b6b8-43d7-a894-5554bb4e04eb">

## Implementation details

Wrap the method in a try catch block, and return default on that invalid
connection string.
The caller method already returns `default` in other cases.

## Test coverage

This is an small fix, I didn't write a test case for this... yet.

## Other details

Backport of #5956

Co-authored-by: Andrew Lock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) identified-by:telemetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants