-
Notifications
You must be signed in to change notification settings - Fork 829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented base zPages classes and TraceZ zPage #1380
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1380 +/- ##
============================================
+ Coverage 91.94% 92.00% +0.06%
Complexity 890 890
============================================
Files 114 114
Lines 3201 3201
Branches 262 262
============================================
+ Hits 2943 2945 +2
+ Misses 176 175 -1
+ Partials 82 81 -1
Continue to review full report at Codecov.
|
summary table generation with running span logic
It looks like something went wrong with this branch, as it's got a TON of things that it shouldn't in it. Seems like you might need to rebase this and re-submit? |
ooh. maybe you got it already. sorry for the noise! |
Yep, we merged our master branch with the OT master branch and cleaned up the old files. |
...ensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanProcessor.java
Outdated
Show resolved
Hide resolved
...ensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanProcessor.java
Outdated
Show resolved
Hide resolved
...tensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandler.java
Outdated
Show resolved
Hide resolved
...extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageHttpHandler.java
Show resolved
Hide resolved
...nsions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezDataAggregator.java
Outdated
Show resolved
Hide resolved
...ensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanProcessor.java
Outdated
Show resolved
Hide resolved
* | ||
* @return a Collection of {@link io.opentelemetry.sdk.trace.ReadableSpan}. | ||
*/ | ||
public Collection<ReadableSpan> getCompletedSpans() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return a copy perhaps?
...xtensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanBuckets.java
Outdated
Show resolved
Hide resolved
...xtensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanBuckets.java
Outdated
Show resolved
Hide resolved
Map<String, TracezSpanBuckets> completedSpanCache = spanProcessor.getCompletedSpanCache(); | ||
TracezSpanBuckets buckets = completedSpanCache.get(spanName); | ||
if (buckets == null) { | ||
return new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collections.emptyList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an error with immutability vs mutability if we revert back to Collections.emptyList
. Should I change the function signature to return an immutable list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're ok with guava, then yes. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, turns out there's another way to do it as well! You can resolve this comment along with the one below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait to resolve until it's been changed. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I already pushed the change, so I hope it updates in the repo soon.
...ensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanProcessor.java
Outdated
Show resolved
Hide resolved
...sions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TracezSpanBucketsTest.java
Outdated
Show resolved
Hide resolved
...ions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandlerTest.java
Outdated
Show resolved
Hide resolved
Map<String, TracezSpanBuckets> completedSpanCache = spanProcessor.getCompletedSpanCache(); | ||
TracezSpanBuckets buckets = completedSpanCache.get(spanName); | ||
if (buckets == null) { | ||
return new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collections.emptyList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as above
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageLogo.java
Outdated
Show resolved
Hide resolved
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/SpanBucket.java
Outdated
Show resolved
Hide resolved
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageLogo.java
Outdated
Show resolved
Hide resolved
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageLogo.java
Outdated
Show resolved
Hide resolved
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageLogo.java
Show resolved
Hide resolved
sdk_extensions/zpages/src/main/java/io/opentelemetry/sdk/extensions/zpages/ZPageLogo.java
Show resolved
Hide resolved
I don't think I have permission to resolve comments. Let me talk to Terry. |
It doesn't seem like Terry can make me an assignee or give me permission to resolve comments. Are code owners able to do this? |
I can resolve them for you. Just add a message to the end of each conversation you think should be resolved. |
Alright, sounds good. For the open comments right now, Terry and I plan to call and resolve them. |
We've went through all the comments and resolved the ones that we think are resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor points on the test, please take a look but otherwise looks good. Thanks a lot for the help!
...ions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandlerTest.java
Outdated
Show resolved
Hide resolved
...ions/zpages/src/test/java/io/opentelemetry/sdk/extensions/zpages/TracezZPageHandlerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the hard work!
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.concurrent.atomic.AtomicReferenceArray; | ||
|
||
final class SpanBucket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to have unit tests on this class specifically, but that can be in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have 3 follow-on requests for separate PRs after this one:
- Make the ZPageServer not a singleton.
- Add unit tests for the SpanBucket
- Add an index page, so you don't have to remember the
/tracez
url to use this feature.
Thanks for all the hard work on this! |
zPages are a set of in-process dynamically generated HTML pages that collect and display data from the running process. In the set, TraceZ (/tracez) is the zPage that focuses on information collected about the trace spans. In particular, the TraceZ zPage displays information on sample running spans, sample latency, and sample error spans.
This PR implemented:
the base classes that are needed for creating a new zPage
the TraceZ zPage