This repository has been archived by the owner on Oct 3, 2023. It is now read-only.
flush interval reader when stopping metric reader #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix staticcheck error
quentin-cha
force-pushed
the
upgrade_dep
branch
from
72d9183 to
7b0ba26
Compare
quentin-cha
commented
Jan 31, 2021
Makefile
Outdated
| @@ -114,7 +114,7 @@ goimports: | |||
|
|
|||
| .PHONY: staticcheck | |||
| staticcheck: | |||
| $(STATICCHECK) ./... | |||
| $(STATICCHECK) -checks "inherit,-SA1019" ./... | |||
There was a problem hiding this comment.
I added this to bypass SA1019 because the following failure after dependency upgrade:
internal/testpb/test.pb.go:42:9: xxx_messageInfo_FooRequest.Unmarshal is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:45:9: xxx_messageInfo_FooRequest.Marshal is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:48:2: xxx_messageInfo_FooRequest.Merge is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:51:9: xxx_messageInfo_FooRequest.Size is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:54:2: xxx_messageInfo_FooRequest.DiscardUnknown is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:57:32: proto.InternalMessageInfo is deprecated: Do not use; this type existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:87:9: xxx_messageInfo_FooResponse.Unmarshal is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:90:9: xxx_messageInfo_FooResponse.Marshal is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:93:2: xxx_messageInfo_FooResponse.Merge is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:96:9: xxx_messageInfo_FooResponse.Size is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:99:2: xxx_messageInfo_FooResponse.DiscardUnknown is deprecated: Do not use; this method existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:102:33: proto.InternalMessageInfo is deprecated: Do not use; this type existed for intenal-use only. (SA1019)
internal/testpb/test.pb.go:105:2: proto.RegisterType is deprecated: Use protoregistry.GlobalTypes.RegisterMessage instead. (SA1019)
internal/testpb/test.pb.go:106:2: proto.RegisterType is deprecated: Use protoregistry.GlobalTypes.RegisterMessage instead. (SA1019)
internal/testpb/test.pb.go:109:15: proto.RegisterFile is deprecated: Use protoregistry.GlobalFiles.RegisterFile instead. (SA1019)
metrics.go:27:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
metrics_batcher.go:26:2: package cloud.google.com/go/monitoring/apiv3 is deprecated: Please use cloud.google.com/go/monitoring/apiv3/v2. (SA1019)
metrics_batcher_test.go:23:2: package cloud.google.com/go/monitoring/apiv3 is deprecated: Please use cloud.google.com/go/monitoring/apiv3/v2. (SA1019)
metrics_proto_api_test.go:27:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
metrics_test.go:23:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
metrics_utils_test.go:24:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
stats.go:34:2: package cloud.google.com/go/monitoring/apiv3 is deprecated: Please use cloud.google.com/go/monitoring/apiv3/v2. (SA1019)
stats_test.go:23:2: package cloud.google.com/go/monitoring/apiv3 is deprecated: Please use cloud.google.com/go/monitoring/apiv3/v2. (SA1019)
trace.go:25:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
trace_proto_test.go:27:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (SA1019)
Please let me know if this should be an issue or be fix as part of the PR.
There was a problem hiding this comment.
I'd prefer not disabling the deprecated notices. I might consider disabling specific files with //lint directives if the upgrade is problematic for some reason, but only if it isn't possible to use the new package easily.
There was a problem hiding this comment.
I think I have fixed the staticcheck errors. I had to make quite a bit of changes. I did test it by deploying it into Knative cluster just to be sure both metrics and traces are being exported correctly. Please let me know what you think. Thanks again!
quentin-cha
commented
Jan 31, 2021
| @@ -147,6 +145,7 @@ func (e *statsExporter) startMetricsReader() error { | |||
| func (e *statsExporter) stopMetricsReader() { | |||
| if e.ir != nil { | |||
| e.ir.Stop() | |||
| e.ir.Flush() | |||
There was a problem hiding this comment.
This is the actual change
quentin-cha
commented
Feb 2, 2021
| @@ -324,10 +324,18 @@ func TestExportTrace(t *testing.T) { | |||
| if !reflect.DeepEqual(spbs, expectedSpans) { | |||
| var got, want []string | |||
| for _, s := range spbs { | |||
| got = append(got, proto.MarshalTextString(s)) | |||
| bytes, err := prototext.Marshal(s) | |||
There was a problem hiding this comment.
sample failure output:
=== RUN TestExportTrace
trace_proto_test.go:340: got spans:
display_name:{value:"span0"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} same_process_as_parent_span:{}
display_name:{value:"span1"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} same_process_as_parent_span:{value:true}
display_name:{value:"span2"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"custom-agent"}}} attribute_map:{key:"key1" value:{int_value:100}} attribute_map:{key:"key2" value:{string_value:{value:"value2"}}}} time_events:{time_event:{annotation:{description:{value:"in span2"}}} time_event:{annotation:{description:{value:"1/2"}}} time_event:{message_event:{type:SENT id:291 uncompressed_size_bytes:1024 compressed_size_bytes:512}}} same_process_as_parent_span:{value:true}
display_name:{value:"span3"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} time_events:{time_event:{annotation:{description:{value:"in span3"}}} time_event:{message_event:{type:RECEIVED id:1110 uncompressed_size_bytes:2048 compressed_size_bytes:1536}}} status:{code:14} same_process_as_parent_span:{value:true}
display_name:{value:"span4"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} time_events:{time_event:{annotation:{description:{value:"1/2"} attributes:{attribute_map:{key:"k1" value:{string_value:{value:"v1"}}}}}} time_event:{annotation:{description:{value:"foo 42"} attributes:{attribute_map:{key:"k2" value:{string_value:{value:"v2"}}}}}} time_event:{annotation:{description:{value:"in span4"} attributes:{attribute_map:{key:"k3" value:{string_value:{value:"v3"}}}}}}} links:{link:{trace_id:"01020000000000000000000000000000" span_id:"0300000000000000" type:PARENT_LINKED_SPAN attributes:{attribute_map:{key:"k4" value:{string_value:{value:"v4"}}}}}} same_process_as_parent_span:{value:true}
want:
display_name:{value:"span0"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} same_process_as_parent_span:{}
display_name:{value:"span1"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} same_process_as_parent_span:{value:true}
display_name:{value:"span2"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"custom-agent"}}} attribute_map:{key:"key1" value:{int_value:100}} attribute_map:{key:"key2" value:{string_value:{value:"value2"}}}} time_events:{time_event:{annotation:{description:{value:"in span2"}}} time_event:{annotation:{description:{value:"1/2"}}} time_event:{message_event:{type:SENT id:291 uncompressed_size_bytes:1024 compressed_size_bytes:512}}} same_process_as_parent_span:{value:true}
display_name:{value:"span3"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} time_events:{time_event:{annotation:{description:{value:"in span3"}}} time_event:{message_event:{type:RECEIVED id:1110 uncompressed_size_bytes:2048 compressed_size_bytes:1536}}} status:{code:14} same_process_as_parent_span:{value:true}
display_name:{value:"span4"} attributes:{attribute_map:{key:"g.co/agent" value:{string_value:{value:"opencensus-go 0.23.0; stackdriver-exporter 0.13.3"}}}} time_events:{time_event:{annotation:{description:{value:"1/2"} attributes:{attribute_map:{key:"k1" value:{string_value:{value:"v1"}}}}}} time_event:{annotation:{description:{value:"foo 42"} attributes:{attribute_map:{key:"k2" value:{string_value:{value:"v2"}}}}}} time_event:{annotation:{description:{value:"in span4"} attributes:{attribute_map:{key:"k3" value:{string_value:{value:"v3"}}}}}}} links:{link:{trace_id:"01020000000000000000000000000000" span_id:"0300000000000000" type:PARENT_LINKED_SPAN attributes:{attribute_map:{key:"k4" value:{string_value:{value:"v4"}}}}}} same_process_as_parent_span:{value:true}
--- FAIL: TestExportTrace (0.01s)
dashpole
approved these changes
Feb 3, 2021
| @@ -62,6 +62,8 @@ func (s *testServer) Multiple(stream Foo_MultipleServer) error { | |||
| } | |||
| } | |||
|
|
|||
| func (*testServer) mustEmbedUnimplementedFooServer() {} | |||
There was a problem hiding this comment.
a comment explaining why this is needed would be useful.
metrics_proto_api_test.go
Outdated
| @@ -430,7 +428,8 @@ func readTestCaseFromFiles(t *testing.T, filename string) *testCases { | |||
| strMetrics := strings.Split(string(f), "---") | |||
| for _, strMetric := range strMetrics { | |||
| in := metricspb.Metric{} | |||
| err = proto.UnmarshalText(strMetric, &in) | |||
| //err = proto.UnmarshalText(strMetric, &in) | |||
There was a problem hiding this comment.
remove commented out code.
quentin-cha
force-pushed
the
upgrade_dep
branch
from
1fcae08 to
b7730f9
Compare
|
Thanks again for helping me out! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Flush interval reader when stopping metric reader. Fixes unit test after update dependencies.