gcp/observability: Implement tracing/metrics via OpenCensus #5372
Conversation
|
The flake looks unrelated, filed #5375 |
|
Looks like there are compilation errors here now:
|
|
Good catch 😵 I'll rebase to clean up the commits once the other two PRs are merged into master. |
|
@dfawley PTAL. This PR is now up-to-date with master branch. |
| deadline := time.Now().Add(defaultTestTimeout) | ||
| for time.Now().Before(deadline) { |
There was a problem hiding this comment.
FWIW IMO it's nicer to use a context for things like this.
ctx, cancel := context.WithTimeout(context.Backgound(), defaultTestTimeout)
defer cancel()
for ctx.Err() == nil {
...The reason for this is that contexts are more versatile (you might need to pass one into function calls anyway), so you may already have one, and it's good to follow a consistent pattern.
That said this is fine, so no need to change.
There was a problem hiding this comment.
Updated, this way feels more Golang. I didn't find the second usage of the new context though.
TIL if a context is timed out, the timeout error will be in Err().
There was a problem hiding this comment.
I didn't find the second usage of the new context though.
That's why I was saying no need to change for this PR; there is no other context usage here. Many tests need contexts already for other purposes, so using that same context for a loop timeout works well.
ctx.Err() is either Canceled or DeadlineExceeded or nil.
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| if !validClientViews || !validServerViews || !validSpans { | ||
| t.Fatalf("Invalid OpenCensus export data: validClientViews=%v validServerViews=%v validSpans=%v", validClientViews, validServerViews, validSpans) |
There was a problem hiding this comment.
The problem with this is it doesn't show what values were received instead.
IMO it would be better to save the actual values and log them here.
There was a problem hiding this comment.
Done. Using error as the recording type instead of bool.
| deadline := time.Now().Add(defaultTestTimeout) | ||
| for time.Now().Before(deadline) { |
There was a problem hiding this comment.
Updated, this way feels more Golang. I didn't find the second usage of the new context though.
TIL if a context is timed out, the timeout error will be in Err().
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| if !validClientViews || !validServerViews || !validSpans { | ||
| t.Fatalf("Invalid OpenCensus export data: validClientViews=%v validServerViews=%v validSpans=%v", validClientViews, validServerViews, validSpans) |
There was a problem hiding this comment.
Done. Using error as the recording type instead of bool.
|
All checks are passing, PTALA. @dfawley |
| deadline := time.Now().Add(defaultTestTimeout) | ||
| for time.Now().Before(deadline) { |
There was a problem hiding this comment.
I didn't find the second usage of the new context though.
That's why I was saying no need to change for this PR; there is no other context usage here. Many tests need contexts already for other purposes, so using that same context for a loop timeout works well.
ctx.Err() is either Canceled or DeadlineExceeded or nil.
| validClientViews = false | ||
| validServerViews = false | ||
| validSpans = false | ||
| var clientViewsError, serverViewsError, spansError error |
There was a problem hiding this comment.
How about var errs []error? It might be simpler. (Or []string would work for this, too.)
var errs []error
for ctx.Err() == nil {
errs = nil
if ...; value != TypeOpenCensusViewCount {
errs = append(errs, fmt.Errorf("this error %v", value))
}
...
if len(errs) == 0 {
break
}
}
if len(errs) > 0 {
t.Fatalf("Invalid OpenCensus export data: %v", errs)
}
dfawley
changed the title
As title.
This PR includes changes from #5352 and #5347
RELEASE NOTES: