-
Notifications
You must be signed in to change notification settings - Fork 510
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
Cliquery #2775
Cliquery #2775
Conversation
spanCount := 0 | ||
firstStartTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].StartTimeUnixNano | ||
lastEndTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].EndTimeUnixNano | ||
rootSpan := combinedTrace.Batches[0].ScopeSpans[0].Spans[0] |
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.
the root span is not always the first. as you're iterating through all the spans below look for one with no parent span id
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.
instead of grabbing this first span like this we should init firstStartTime/lastEndTime/rootSpan/rootResource to better first values. something like this diff:
@@ -4,11 +4,14 @@ import (
"bytes"
"context"
"fmt"
+ "math"
"sort"
"github.com/gogo/protobuf/jsonpb"
"github.com/grafana/tempo/pkg/model/trace"
+ v1resource "github.com/grafana/tempo/pkg/tempopb/resource/v1"
+ v1 "github.com/grafana/tempo/pkg/tempopb/trace/v1"
"github.com/grafana/tempo/pkg/util"
)
@@ -47,15 +50,16 @@ func (cmd *queryBlocksSummaryCmd) Run(ctx *globalOptions) error {
}
combinedTrace, _ := combiner.Result()
+
+ var rootSpan *v1.Span
+ var rootSpanResource *v1resource.Resource
size := 0
spanCount := 0
- firstStartTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].StartTimeUnixNano
- lastEndTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].EndTimeUnixNano
- rootSpan := combinedTrace.Batches[0].ScopeSpans[0].Spans[0]
- rootSpanResource := combinedTrace.Batches[0].Resource
+ firstStartTime := uint64(math.MaxUint64)
+ lastEndTime := uint64(0)
+
rootServiceName := ""
serviceNameMap := make(map[string]int)
-
for _, b := range combinedTrace.Batches {
size += b.Size()
for _, attr := range b.Resource.Attributes {
|
||
// get top 5 most frequent service names | ||
lowest := 0 | ||
topFiveName := make([]string, 5) |
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 would probably just put all of the key/value pairs in a slice and sort it. it won't be as efficient, but it will be clearer and this CLI command doesn't need to be super efficient.
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.
heading the right way. we need:
- changelog
- docs
i also don't like the command name, but it does nicely match the query blocks command.
spanCount := 0 | ||
firstStartTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].StartTimeUnixNano | ||
lastEndTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].EndTimeUnixNano | ||
rootSpan := combinedTrace.Batches[0].ScopeSpans[0].Spans[0] |
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.
instead of grabbing this first span like this we should init firstStartTime/lastEndTime/rootSpan/rootResource to better first values. something like this diff:
@@ -4,11 +4,14 @@ import (
"bytes"
"context"
"fmt"
+ "math"
"sort"
"github.com/gogo/protobuf/jsonpb"
"github.com/grafana/tempo/pkg/model/trace"
+ v1resource "github.com/grafana/tempo/pkg/tempopb/resource/v1"
+ v1 "github.com/grafana/tempo/pkg/tempopb/trace/v1"
"github.com/grafana/tempo/pkg/util"
)
@@ -47,15 +50,16 @@ func (cmd *queryBlocksSummaryCmd) Run(ctx *globalOptions) error {
}
combinedTrace, _ := combiner.Result()
+
+ var rootSpan *v1.Span
+ var rootSpanResource *v1resource.Resource
size := 0
spanCount := 0
- firstStartTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].StartTimeUnixNano
- lastEndTime := combinedTrace.Batches[0].ScopeSpans[0].Spans[0].EndTimeUnixNano
- rootSpan := combinedTrace.Batches[0].ScopeSpans[0].Spans[0]
- rootSpanResource := combinedTrace.Batches[0].Resource
+ firstStartTime := uint64(math.MaxUint64)
+ lastEndTime := uint64(0)
+
rootServiceName := ""
serviceNameMap := make(map[string]int)
-
for _, b := range combinedTrace.Batches {
size += b.Size()
for _, attr := range b.Resource.Attributes {
@ie-pham This looks like a useful tool. Should we add some doc for this? |
// get top 5 most frequent service names | ||
topFiveSortedPL := sortServiceNames(serviceNameMap) | ||
topFiveServiceName := make([]string, 5) | ||
length := len(topFiveSortedPL) |
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.
what if length is less than 5? i think this panics
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.
since we are iterating through the actual list of service names using its length (or if it's longer than 5 use 5), even if it's less than 3, we wouldn't get to out of range
. It just prints out a weird looking array with extra spaces
[a b c ]
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]