[SVLS-3545] Add Serverless metric origins to dogstatsd package #876
+301
−5
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
dogstatsd/src/metric.rs
Outdated
| self.values.iter().any(|(k, _)| k.as_str() == key) | ||
| } | ||
|
|
||
| pub fn get(&self, key: &str) -> Option<&str> { |
There was a problem hiding this comment.
this assumes that one key correspond to one value, which is conceptually wrong because:
- some keys have no value
- keys are not unique, in fact it's a vector and not a dictoinary/map
a find_all might be better, but it depends on why it is needed
There was a problem hiding this comment.
I'm using this to check if a tag like origin:containerapp exists in order to identify the metric origin, so I'll want to see if that key is available and then access the value. I added a check on whether the value exists, but curious if you think that's enough?
There was a problem hiding this comment.
it is a bit misleading to create a method for that, the case when it's only one element with that name is a corner case.
I think the test and the code should handle the general case too
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
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