-
Notifications
You must be signed in to change notification settings - Fork 8
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
Health Metrics for data-pipeline #618
Conversation
@@ -1,4 +1,4 @@ | |||
// Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ | |||
// Copyright 2024-Present Datadog, Inc. https://www.datadoghq.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue what the correct answer is, but should we be modifying the license headers like this? I'm not sure what is legally correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc it doesn't strictly matter as long as the header is there when the file is created
data-pipeline/src/trace_exporter.rs
Outdated
@@ -213,23 +231,36 @@ impl TraceExporter { | |||
return Ok(String::from("{}")); | |||
} | |||
|
|||
// todo: do we need to modify the client to allow for &str to avoid allocating a String? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client should probably have some sort of ActionType
enum implemented instead of passing strings like this. Partially for the allocation cost, and partially to reduce the likelihood of typos.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
==========================================
+ Coverage 73.05% 73.28% +0.23%
==========================================
Files 252 252
Lines 36093 36153 +60
==========================================
+ Hits 26366 26496 +130
+ Misses 9727 9657 -70
|
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
BenchmarksComparisonBenchmark execution time: 2024-09-17 18:39:54 Comparing candidate commit 5532b66 in PR branch Found 7 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:normalization/normalize_trace/test_trace
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
ae5c14e
to
8fecb9a
Compare
Closing this PR in favor of this other branch where I split the implementation which is much simpler to read (at the cost of some code duplication) https://github.com/DataDog/libdatadog/pull/638/files |
What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.