Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Instrumentation/HTTP: Handle incoming requests with long request url … #515

Merged
merged 2 commits into from
May 10, 2019
Merged

Instrumentation/HTTP: Handle incoming requests with long request url … #515

merged 2 commits into from
May 10, 2019

Conversation

mayurkale22
Copy link
Member

…path

Fixes #486

@mayurkale22 mayurkale22 mentioned this pull request May 10, 2019
Closed
@codecov-io
Copy link

codecov-io commented May 10, 2019 •
edited
Loading

Codecov Report

Merging #515 into master will increase coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
+ Coverage   94.81%   95.17%   +0.35%     
==========================================
  Files         147      147              
  Lines       10604    10582      -22     
  Branches      905      899       -6     
==========================================
+ Hits        10054    10071      +17     
+ Misses        550      511      -39
Impacted Files Coverage Δ
src/detect-resource.ts 90.9% <0%> (-1.95%) ⬇️
src/common-utils.ts 95.83% <0%> (-0.84%) ⬇️
test/test-detect-resource.ts 99.13% <0%> (-0.38%) ⬇️
src/http.ts 91.6% <0%> (-0.34%) ⬇️
src/tracecontext-format.ts 96.42% <0%> (-0.19%) ⬇️
test/test-tracecontext-format.ts 99.06% <0%> (-0.04%) ⬇️
src/http-stats.ts 100% <0%> (ø) ⬆️
src/grpc-stats/server-stats.ts 100% <0%> (ø) ⬆️
src/grpc-stats/client-stats.ts 100% <0%> (ø) ⬆️
src/grpc-stats/stats-common.ts 100% <0%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 727b59c...9d5cb11. Read the comment docs.

@mayurkale22 mayurkale22 added this to the Release 0.0.12 milestone May 10, 2019
@vmarchaud
Copy link
Contributor

I'm not sure if they are other places where we set tag value that may be above the 256 length mark.

@mayurkale22
Copy link
Member Author

I'm not sure if they are other places where we set tag value that may be above the 256 length mark.

Other places are for client/server > method and client/server > statusCode, IMO they don't have this problem.

draffensperger
draffensperger approved these changes May 10, 2019
View reviewed changes
packages/opencensus-instrumentation-http/src/http.ts Outdated
@@ -228,7 +228,12 @@ export class HttpPlugin extends BasePlugin {
rootSpan.addAttribute(
HttpPlugin.ATTRIBUTE_HTTP_ROUTE, requestUrl.path || '');
tags.set(
stats.HTTP_SERVER_ROUTE, {value: requestUrl.path || ''},
stats.HTTP_SERVER_ROUTE, {
value: ((requestUrl.path && requestUrl.path.length > 255) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified to just (requestUrl.path || '').substring(0, 255)?

Calling substring on a shorter string just returns the string so for strings shorter than 255 that is fine.

Should 255 be a constant as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done. I was thinking not to use substring unless it is really required by checking the length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - I'd say either is fine. I'm not sure whether the substring has a performance/memory penalty, but if it did that seems like a reason to only do it if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, already made the change as per your suggestion.

@mayurkale22 mayurkale22 force-pushed the handle_long_request_url_path branch from c6834ba to 9d5cb11 Compare May 10, 2019 20:50
@mayurkale22 mayurkale22 merged commit aeb5fd4 into census-instrumentation:master May 10, 2019
@mayurkale22 mayurkale22 deleted the handle_long_request_url_path branch May 10, 2019 21:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@draffensperger draffensperger draffensperger approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@justindsmith justindsmith Awaiting requested review from justindsmith

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
Release 0.0.12
Development

Successfully merging this pull request may close these issues.

http instrumentation cannot handle incoming requests with long request url path (>255)
5 participants
@mayurkale22 @codecov-io @vmarchaud @draffensperger @googlebot