Changed TraceConfigz zPage form to use POST request #1521

wangty27 · 2020-08-07T17:00:11Z

This PR addresses #1517

…t tests

…ntation

* Scaffolded logic for basic benchmark tests * Wrote benchmark tests for TracezSpanBuckets * Updated README with benchmark tests * Changed the wording slightly

* Scaffolded logic for basic benchmark tests * Wrote benchmark tests for TracezSpanBuckets * Updated README with benchmark tests * Changed the wording slightly * Added a set of benchmark tests for TracezDataAggregator * Modified README formatting * Changed benchmark test to negate dead code elimination

…DataAggregator benchmark tests

…ions/zpages/TracezDataAggregatorBenchmark.java Co-authored-by: Anuraag Agrawal <[email protected]>

…to master

codecov · 2020-08-07T17:14:08Z

Codecov Report

Merging #1521 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1521   +/-   ##
=========================================
  Coverage     87.07%   87.07%           
  Complexity     1367     1367           
=========================================
  Files           162      162           
  Lines          5191     5191           
  Branches        490      490           
=========================================
  Hits           4520     4520           
  Misses          492      492           
  Partials        179      179

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 5b4e0a8...95ff5b7. Read the comment docs.

v-y-l

Why was this change necessary?

...nsions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandlerTest.java

… on POST request only

anuraaga

Thanks for the quick turnaround!

...extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandler.java

...pages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TraceConfigzZPageHandlerTest.java

…st only

jkwatson · 2020-08-10T17:14:52Z

Why was this change necessary?

The originating issue is #1517, but I'm not convinced this is really an issue. It would seems to be a bad idea to expose your zPages to the public internet, whether via GET or POST, so I'm not really sure this is a big deal. From a pure HTTP semantics perspective, though, GET shouldn't be being used for mutating operations of any kind, so I think from that perspective, it's a good change.

...tensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandler.java

...extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandler.java

anuraaga · 2020-08-10T23:53:57Z

The originating issue is #1517, but I'm not convinced this is really an issue. It would seems to be a bad idea to expose your zPages to the public internet, whether via GET or POST, so I'm not really sure this is a big deal.

The page is exposed to someone, probably on an internal network from a browser. Opening a malicious email (not even clicking a link) that happened to know the URL would be enough for them to change the config. Knowing the URL and who to target aren't easy, but we should generally consider these as big deals.

anuraaga · 2020-08-11T11:39:40Z

...ns/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TraceConfigzZPageHandler.java

+      try {
+        applyTraceConfig(queryMap);
+      } catch (Throwable t) {
+        try (PrintStream out = new PrintStream(outputStream, /* autoFlush= */ false, "UTF-8")) {


This section seems like an error applying the config, not an error generating HTML right?

Yea it's error applying config, I will adjust the error message

...extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandler.java

...pages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TraceConfigzZPageHandlerTest.java

…tstream

anuraaga

Thanks!

williamhu99 and others added 30 commits July 18, 2020 00:27

Removed URLEncoder

ec31e3e

Fixed typo

e0fe0ed

Added URLDecoding

cf7f5c8

Included comment for string replacement

9db8e7e

Added unit tests for special characters in span names

4127978

Resolved URL decoding issues

e17c337

Moved url decoding to parseQueryMap and updated the corresponding uni…

bb58b26

…t tests

Added a README file for zPage quickstart

80a5ccf

Add images for README

dcbd9aa

Updated README

24ffce2

Add frontend images

5047849

Add backend images

85a539e

Added our design doc

4156123

Added details on package

265cf27

Reworded a few lines

c102e45

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

e977dcf

…try-java into master

Moved DESIGN.md to a docs folder and changed gradle config to impleme…

ed8ef33

…ntation

Changed wording regarding HttpServer requirement

f595909

Added zpages folder under docs, resolved broken image links

806b632

Resolved comments for the design md file

9e4dde8

Made a few wording changes

86c2a51

Wrote a benchmark test for TracezSpanBuckets (#23)

a3c9a5a

* Scaffolded logic for basic benchmark tests * Wrote benchmark tests for TracezSpanBuckets * Updated README with benchmark tests * Changed the wording slightly

Updated README file (#25)

07b2fcf

Merged with original repo

fe056e3

Added Javadocs to the TracezDataAggregator benchmark tests

f714b43

Removed benchmark results from README and added a param to the Tracez…

922e950

…DataAggregator benchmark tests

Update sdk_extensions/zpages/src/jmh/java/io/opentelemetry/sdk/extens…

13d82c4

…ions/zpages/TracezDataAggregatorBenchmark.java Co-authored-by: Anuraag Agrawal <[email protected]>

Added multiple param values for TracezDataAggregatorBenchmark

5ab6f6a

Merge branch 'master' of github.com:williamhu99/opentelemetry-java in…

18a4141

…to master

wty27 added 2 commits August 7, 2020 14:36

Merged with OpenTelemetry repo for latest updates

0ad0ebe

Changed TraceConfigz zPage form submit to use POST request

48138f7

wangty27 requested review from anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, thisthat and tylerbenson as code owners August 7, 2020 17:00

v-y-l reviewed Aug 7, 2020

View reviewed changes

...nsions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandlerTest.java Outdated Show resolved Hide resolved

Added requestMethod parameter to emitHtml, limited TraceConfig change…

1eb5332

… on POST request only

anuraaga reviewed Aug 8, 2020

View reviewed changes

Removed duplicate parse function, added test for update on POST reque…

0f09288

…st only

jkwatson reviewed Aug 10, 2020

View reviewed changes

...tensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandler.java Outdated Show resolved Hide resolved

jkwatson reviewed Aug 10, 2020

View reviewed changes

...extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandler.java Outdated Show resolved Hide resolved

Added separate method for processing request

8044ba1

anuraaga reviewed Aug 11, 2020

View reviewed changes

Removed unnecessary error check in tests, used try resources for inpu…

95ff5b7

…tstream

anuraaga approved these changes Aug 12, 2020

View reviewed changes

carlosalberto approved these changes Aug 12, 2020

View reviewed changes

jkwatson approved these changes Aug 12, 2020

View reviewed changes

jkwatson merged commit ce8cfd4 into open-telemetry:master Aug 12, 2020

wangty27 deleted the fix-get-request branch August 13, 2020 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed TraceConfigz zPage form to use POST request #1521

Changed TraceConfigz zPage form to use POST request #1521

wangty27 commented Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

Loading

v-y-l left a comment

anuraaga left a comment

jkwatson commented Aug 10, 2020

anuraaga commented Aug 10, 2020

anuraaga Aug 11, 2020

wangty27 Aug 11, 2020

anuraaga left a comment

Changed TraceConfigz zPage form to use POST request #1521

Changed TraceConfigz zPage form to use POST request #1521

Conversation

wangty27 commented Aug 7, 2020

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

v-y-l left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

jkwatson commented Aug 10, 2020

anuraaga commented Aug 10, 2020

anuraaga Aug 11, 2020

Choose a reason for hiding this comment

wangty27 Aug 11, 2020

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2020 •

edited

Loading