[SVLS-3545] Add Serverless metric origins to dogstatsd package #876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can we get Azure Functions in on this? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
- Coverage 71.90% 71.74% -0.17%
==========================================
Files 324 329 +5
Lines 48077 48695 +618
==========================================
+ Hits 34572 34936 +364
- Misses 13505 13759 +254
|
Yup, we can do this but will require some updates on the backend before it's available. |
alexgallotta
requested changes
Feb 13, 2025
BenchmarksComparisonBenchmark execution time: 2025-02-20 18:54:47 Comparing candidate commit 1e36f6f in PR branch Found 1 performance improvements and 45 performance regressions! Performance is the same for 6 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
scenario:benching string interning on wordpress profile
scenario:concentrator/add_spans_to_concentrator
scenario:credit_card/is_card_number/
scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/37828224631
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:ip_address/quantize_peer_ip_address_benchmark
scenario:normalization/normalize_name/normalize_name/Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Lo...
scenario:normalization/normalize_name/normalize_name/bad-name
scenario:normalization/normalize_name/normalize_name/good
scenario:normalization/normalize_service/normalize_service/A0000000000000000000000000000000000000000000000000...
scenario:normalization/normalize_service/normalize_service/Data🐨dog🐶 繋がっ⛰てて
scenario:normalization/normalize_service/normalize_service/Test Conversion 0f Weird !@#$%^&**() Characters
scenario:normalization/normalize_service/normalize_service/[empty string]
scenario:normalization/normalize_trace/test_trace
scenario:redis/obfuscate_redis_string
scenario:sql/obfuscate_sql_string
scenario:tags/replace_trace_tags
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
Group 13
BaselineOmitted due to size. |
Comment on lines
+133
to
+142
| tags.find_all(key) | ||
| .iter() | ||
| .filter_map(|value| { | ||
| if !value.is_empty() { | ||
| Some(value.as_str()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .next() |
There was a problem hiding this comment.
@alexgallotta I implemented the find_all() method for SortedTags and am now filtering down to the first valid tag value from the found tags here where we handle origin detection
There was a problem hiding this comment.
I think this could be trickier than it looks like, and it's better to be more explicit on the assumptions so we don't miss potential bugs:
- is this tag expected to be unique?
- what happens if there is more than one tag like that?
- can this tag exists without a value?
Assuming that the "happy path" would have this tag unique and with value:
- find all the matching tags
- if more than one is found, pick the first one but at least log a warning with the list of tags that were found
- if the tag is found but it has no value, it is possibly wrong or a bug
so the function should be something like "find_unique_tag_with_value"
The main goal is to make it very clear in the code on how this tag should look like, and at least warn if something is not as expected.
Customers can set arbitrary tags, so:
- could add multiple tags with that key, and silently break this
- there could be a tag key with a blank string.
is_emptycheck only the length, so " " is not empty
there is probably more corner cases, and logging a warning in the non happy path could help debug issue if something weird happens
| if is_datadog_metric(&prefix) { | ||
| return None; | ||
| } | ||
| if is_azure_app_services(&tags, &prefix) { |
There was a problem hiding this comment.
Some duplicate code could be removed with an approach like
fn serverless_origin(category: Category) -> Origin {
Origin {
origin_product: OriginProduct::Serverless.into(),
origin_service: OriginService::Other.into(),
origin_category: category.into(),
..Default::default()
}
}and
if is_azure_app_services(&tags, &prefix) {
return Some(serverless_origin(OriginCategory::AppServicesMetrics.into()));
}
Comment on lines
+133
to
+142
| tags.find_all(key) | ||
| .iter() | ||
| .filter_map(|value| { | ||
| if !value.is_empty() { | ||
| Some(value.as_str()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .next() |
There was a problem hiding this comment.
I think this could be trickier than it looks like, and it's better to be more explicit on the assumptions so we don't miss potential bugs:
- is this tag expected to be unique?
- what happens if there is more than one tag like that?
- can this tag exists without a value?
Assuming that the "happy path" would have this tag unique and with value:
- find all the matching tags
- if more than one is found, pick the first one but at least log a warning with the list of tags that were found
- if the tag is found but it has no value, it is possibly wrong or a bug
so the function should be something like "find_unique_tag_with_value"
The main goal is to make it very clear in the code on how this tag should look like, and at least warn if something is not as expected.
Customers can set arbitrary tags, so:
- could add multiple tags with that key, and silently break this
- there could be a tag key with a blank string.
is_emptycheck only the length, so " " is not empty
there is probably more corner cases, and logging a warning in the non happy path could help debug issue if something weird happens
| } | ||
|
|
||
| fn is_google_cloud_run(tags: &SortedTags, prefix: &str) -> bool { | ||
| get_first_tag_value(tags, DD_ORIGIN_TAG_KEY) == Some(GOOGLE_CLOUD_RUN_TAG_VALUE) |
There was a problem hiding this comment.
Wrapping the result strings on Some just for comparison feels wrong.
You can change it with map_or, default to false
get_first_tag_value(tags, DD_ORIGIN_TAG_KEY).map_or(false, |v| v == GOOGLE_CLOUD_RUN_TAG_VALUE)
|
|
||
| fn is_azure_app_services(tags: &SortedTags, prefix: &str) -> bool { | ||
| get_first_tag_value(tags, DD_ORIGIN_TAG_KEY) == Some(AZURE_APP_SERVICES_TAG_VALUE) | ||
| && prefix != AZURE_APP_SERVICES_PREFIX |
There was a problem hiding this comment.
nitpick, in general it's always better to do CHEAP_CHECK && EXPENSIVE_CHECK
since the tag search is iterating over N tags, it is arguably better to switch the other with
prefix != AZURE_APP_SERVICES_PREFIX
&& TAG_ESXPENSIVE_CHECK
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds metric origins for Serverless. This will effect the Lambda Extension and Serverless Compatibility Layer.
Currently all metrics (custom and standard) are bucketed into the
serverlessorigin product. At the moment, we just want to be able to differentiate custom metrics by their source. Reference this mapping for Serverless metric origin designations.Motivation
SVLS-3545 - correct Serverless metric origins
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Copy the git hash to your Cargo.toml