Fix URI path in EnrichedSpanUtils#197
Conversation
pavan-traceable
requested review from
a team,
findingrish,
skjindal93 and
surajpuvvada
Codecov Report
@@ Coverage Diff @@
## main #197 +/- ##
============================================
- Coverage 79.51% 79.50% -0.01%
+ Complexity 1133 1131 -2
============================================
Files 101 101
Lines 4370 4363 -7
Branches 406 405 -1
============================================
- Hits 3475 3469 -6
Misses 704 704
+ Partials 191 190 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| return Optional.empty(); | ||
| String httpUrl = getStringAttribute(event, EnrichedSpanConstants.getValue(Http.HTTP_URL)); |
There was a problem hiding this comment.
Shouldn't we get rid of reading everything off from first class fields, at least for this method? If the intention is to make this more of a safer change as a fallback, we should be doing the same thing here
But, I feel if the intention is to get rid of the first class fields, then let's move towards it, and completely get rid of reading the values from first class fields
There was a problem hiding this comment.
There is already ongoing effort to clean up all the fields, if i have completely get rid of first class completely then all the logic HttpFieldsGenerator should be copied to here as well, which i don't want to do it. In case if that logic not able to find http url, we can fallback to checking the attribute map which i am doing here.
There was a problem hiding this comment.
Removal of first class fields is tracked under this issue hypertrace/hypertrace#245, idea is to move logic for populating first class field to a common lib, consumers can then either directly read attributes or leverage this lib
There was a problem hiding this comment.
if i have completely get rid of first class completely then all the logic HttpFieldsGenerator should be copied to here as well
Basically, I have couple of concerns
- It's not enough to just read the URLs from
http.urlattribute. It can also be present inhttp.request.urlattribute. That's why there is an entire list of attributes here https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java#L78 - We should also validate that it's a full URL, and not a half baked, malformed URL, which is the check here https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java#L432. We should not blindly assume that the attributes will have the URL in the proper format
There was a problem hiding this comment.
Got it. Fixed please check now.
| HTTP_REQUEST_QUERY_PARAM = 2 [(string_value) = "http.request.query.param"]; | ||
| HTTP_HOST = 3 [(string_value) = "HTTP_HOST"]; // We should probably stick to a standard of keeping all enriched attributes in capital letters. | ||
| HTTP_REQUEST_PATH_PARAM = 4 [(string_value) = "http.request.path.param"]; | ||
| HTTP_URL = 5 [(string_value) = "http.url"]; |
There was a problem hiding this comment.
http.url isn't an enriched attribute. It's a raw span attribute. There is no need to add it here
There was a problem hiding this comment.
It is already present here https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/raw-span-constants/src/main/proto/org/hypertrace/core/span/constants/v1/span_attribute.proto#L48
Can be accessed, RawSpanConstants.getValue(Http.HTTP_URL)
| } | ||
|
|
||
| return Optional.empty(); | ||
| String httpUrl = getStringAttribute(event, EnrichedSpanConstants.getValue(Http.HTTP_URL)); |
There was a problem hiding this comment.
nit: this var is also not needed
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 participants
Description
While retrieving http url it should also check attribute map apart from first class fields
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.