Add static check and fix all errors #218

songy23 · 2019-08-01T00:16:03Z

Fixes #155.

This is inspired by googleapis/google-cloud-go.

songy23 · 2019-08-01T00:29:04Z

I'm still hesitant about adding this check. IMO most suggestions from this tool are kind of nitpicking. For example here's a sample output:

$ make staticcheck
staticcheck ./...
config/config.go:179:44: should use make(configmodels.Receivers) instead (S1019)
config/config.go:245:44: should use make(configmodels.Exporters) instead (S1019)
config/config.go:302:46: should use make(configmodels.Processors) instead (S1019)
config/config.go:359:44: should use make(configmodels.Pipelines) instead (S1019)
exporter/exporterhelper/metricshelper_test.go:97:68: should use make([]*metricspb.Metric, 1) instead (S1019)
exporter/exporterhelper/tracehelper_test.go:135:33: should use make([]*tracepb.Span, 2) instead (S1019)
exporter/exporterhelper/tracehelper_test.go:154:60: should use make([]*tracepb.Span, 1) instead (S1019)
exporter/exportertest/nop_exporter_test.go:34:5: don't use Yoda conditions (ST1017)
exporter/exportertest/nop_exporter_test.go:48:5: don't use Yoda conditions (ST1017)
exporter/exportertest/nop_exporter_test.go:61:5: don't use Yoda conditions (ST1017)
exporter/exportertest/nop_exporter_test.go:75:5: don't use Yoda conditions (ST1017)
exporter/exportertest/sink_exporter_test.go:42:5: don't use Yoda conditions (ST1017)
exporter/exportertest/sink_exporter_test.go:63:5: don't use Yoda conditions (ST1017)
exporter/jaegerexporter/jaeger_thrift_http_sender.go:114:20: error strings should not be capitalized (ST1005)
exporter/loggingexporter/logging_exporter_test.go:37:5: don't use Yoda conditions (ST1017)
exporter/loggingexporter/logging_exporter_test.go:53:5: don't use Yoda conditions (ST1017)
exporter/opencensusexporter/opencensus.go:60:2: field counter is unused (U1000)
exporter/prometheusexporter/factory_test.go:44:8: const defaultTestEndPoint is unused (U1000)
exporter/zipkinexporter/zipkin.go:110:35: error strings should not be capitalized (ST1005)
internal/collector/processor/idbatcher/id_batcher.go:70:2: field numBatches is unused (U1000)
internal/collector/processor/idbatcher/id_batcher.go:131:3: the surrounding loop is unconditionally terminated (SA4004)
internal/collector/processor/idbatcher/id_batcher_test.go:99:2: the goroutine calls T.Fatal, which must be called in the same goroutine as the test (SA2002)
internal/collector/processor/idbatcher/id_batcher_test.go:111:5: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
internal/collector/processor/idbatcher/id_batcher_test.go:157:24: should use make([][]byte, numIds) instead (S1019)
internal/collector/processor/metrics.go:58:3: redundant break statement (S1023)
observability/observabilitytest/observabilitytest.go:93:20: error strings should not be capitalized (ST1005)
observability/observabilitytest/observabilitytest.go:102:22: error strings should not be capitalized (ST1005)
observability/observabilitytest/observabilitytest.go:108:19: error strings should not be capitalized (ST1005)
processor/addattributesprocessor/addattributesprocessor.go:30:33: error strings should not be capitalized (ST1005)
processor/attributekeyprocessor/attributekeyprocessor_test.go:161:34: should use make([]*tracepb.Span, 1) instead (S1019)
processor/attributekeyprocessor/attributekeyprocessor_test.go:165:35: should use make([]*tracepb.Span, 1) instead (S1019)
processor/attributekeyprocessor/config_test.go:30:27: this value of err is never used (SA4006)
processor/attributekeyprocessor/config_test.go:78:27: this value of err is never used (SA4006)
processor/attributekeyprocessor/factory.go:47:43: should use make(map[string]NewKeyProperties) instead (S1019)
processor/nodebatcher/node_batcher.go:75:2: field bucketMu is unused (U1000)
processor/probabilisticsampler/config_test.go:56:27: this value of err is never used (SA4006)
processor/tailsampling/processor.go:72:2: const sourceFormat is unused (U1000)
processor/tailsampling/processor.go:204:49: should use make([]sampling.Decision, lenPolicies) instead (S1019)
processor/tailsampling/processor_test.go:216:29: should use make([][]byte, numIds) instead (S1019)
processor/tailsampling/processor_test.go:244:6: func newTestPolicy is unused (U1000)
receiver/jaegerreceiver/trace_receiver.go:74:2: field agentServer is unused (U1000)
receiver/jaegerreceiver/trace_receiver.go:96:2: const defaultZipkinThriftUDPPort is unused (U1000)
receiver/jaegerreceiver/trace_receiver_test.go:108:8: this value of err is never used (SA4006)
receiver/jaegerreceiver/trace_receiver_test.go:109:2: should check returned error before deferring conn.Close() (SA5001)
receiver/opencensusreceiver/octrace/opencensus.go:157:2: field tes is unused (U1000)
receiver/opencensusreceiver/opencensus.go:79:25: error strings should not be capitalized (ST1005)
receiver/opencensusreceiver/options.go:98:2: redundant return statement (S1023)
receiver/prometheusreceiver/internal/metricsbuilder.go:55:2: field ts is unused (U1000)
receiver/prometheusreceiver/internal/metricsbuilder_test.go:201:3: field processErr is unused (U1000)
receiver/prometheusreceiver/metrics_receiver.go:50:2: field ocaStore is unused (U1000)
receiver/prometheusreceiver/metrics_receiver.go:60:2: var errAlreadyStarted is unused (U1000)
receiver/prometheusreceiver/metrics_receiver.go:61:2: var errNilMetricsReceiverSink is unused (U1000)
receiver/vmmetricsreceiver/metrics_receiver.go:27:2: var errNilMetricsConsumer is unused (U1000)
receiver/zipkinreceiver/trace_receiver.go:341:9: this value of err is never used (SA4006)
receiver/zipkinreceiver/trace_receiver_test.go:128:3: field want is unused (U1000)
service/builder/pipelines_builder_test.go:71:16: this value of err is never used (SA4006)
service/builder/pipelines_builder_test.go:142:13: this value of err is never used (SA4006)
service/builder/receivers_builder_test.go:105:16: this value of err is never used (SA4006)
service/builder/receivers_builder_test.go:106:22: this value of err is never used (SA4006)
service/builder/receivers_builder_test.go:222:16: this value of err is never used (SA4006)
service/builder/receivers_builder_test.go:223:22: this value of err is never used (SA4006)
service/service.go:48:2: field processor is unused (U1000)
service/service.go:49:2: field receivers is unused (U1000)
service/service.go:195:25: func (*Application).shutdownReceivers is unused (U1000)
service/service_test.go:54:2: the goroutine calls T.Fatalf, which must be called in the same goroutine as the test (SA2002)
service/service_test.go:96:30: should use make([]string, numAddresses) instead (S1019)
translator/trace/jaeger/constants.go:38:2: var errNilTraceID is unused (U1000)
translator/trace/jaeger/constants.go:39:2: var errWrongLenTraceID is unused (U1000)
translator/trace/jaeger/constants.go:41:2: var errNilID is unused (U1000)
translator/trace/jaeger/constants.go:42:2: var errWrongLenID is unused (U1000)
translator/trace/jaeger/jaegerthrift_to_protospan_test.go:109:8: const numOfFiles is unused (U1000)
translator/trace/jaeger/protospan_to_jaegerproto.go:458:12: this value of err is never used (SA4006)
translator/trace/jaeger/protospan_to_jaegerproto.go:466:26: error strings should not be capitalized (ST1005)
translator/trace/jaeger/protospan_to_jaegerthrift.go:165:26: error strings should not be capitalized (ST1005)
make: *** [staticcheck] Error 1

The strictcheck tool doesn't provide an auto-fix option. It seems trivial to fix all these errors. However on the other hand Google-Cloud-Go and gRPC-Go both enforces this check.

A few alternatives I can think of here:

Include this check, print the output but don't make it required.
Only check a few rules. See gRPC-Go vet.sh.

WDYT?

songy23 · 2019-08-01T00:29:29Z

Also cc @odeke-em @rghetia.

rghetia · 2019-08-01T06:19:15Z

It may appear annoying at first, especially because you have lots of nitpicks when you run it for the first time. But subsequently there may be fewer errors to fix on incremental changes. Ideally it would have been nice if it also auto corrected. May be there will be such option in future.

pjanotti · 2019-08-01T06:31:15Z

I like the idea, I'm just not sure if we should do it before milestone 0.1.0 or not.

songy23 · 2019-08-01T16:33:51Z

IMO this shouldn't be a blocker for v0.1.0 release because it's only affecting code styles rather than functionality.

pjanotti · 2019-08-01T17:16:36Z

Agreed: it isn't a blocker, I'm just worried about the churn. That said if the fixes are all small/nit changes than it is fine.

pjanotti

Changes to add the tool itself LGTM.

pjanotti

Changes in this batch LGTM.

Fixes open-telemetry#155.

songy23 · 2019-08-01T18:47:00Z

Should all be fixed - PTAL. Hopefully the bug fixing is an one-off effort. In the future new code will conform to the checks.

codecov-io · 2019-08-01T19:32:43Z

Codecov Report

Merging #218 into master will increase coverage by 0.04%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   72.43%   72.48%   +0.04%     
==========================================
  Files         109      109              
  Lines        6305     6301       -4     
==========================================
  Hits         4567     4567              
+ Misses       1508     1504       -4     
  Partials      230      230

Impacted Files	Coverage Δ
...iver/prometheusreceiver/internal/metricsbuilder.go	`100% <ø> (ø)`	⬆️
receiver/prometheusreceiver/metrics_receiver.go	`64.06% <ø> (ø)`	⬆️
receiver/jaegerreceiver/trace_receiver.go	`77.38% <ø> (ø)`	⬆️
receiver/opencensusreceiver/octrace/opencensus.go	`71.23% <ø> (ø)`	⬆️
internal/collector/processor/metrics.go	`0% <ø> (ø)`	⬆️
receiver/vmmetricsreceiver/metrics_receiver.go	`0% <ø> (ø)`	⬆️
processor/nodebatcher/node_batcher.go	`93.83% <ø> (ø)`	⬆️
...r/addattributesprocessor/addattributesprocessor.go	`100% <ø> (ø)`	⬆️
service/service.go	`69.62% <ø> (+1.51%)`	⬆️
exporter/opencensusexporter/opencensus.go	`68.51% <ø> (ø)`	⬆️
... and 12 more

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 a1ecc6f...28325e4. Read the comment docs.

pjanotti

LGTM. Nice tool addition @songy23

tigrannajaryan · 2019-08-01T23:20:40Z

internal/collector/processor/idbatcher/id_batcher.go

@@ -120,7 +119,7 @@ func (b *batcher) AddToCurrentBatch(id ID) {
 }

 func (b *batcher) CloseCurrentAndTakeFirstBatch() (Batch, bool) {
-	for readBatch := range b.batches {
+	if readBatch, ok := <-b.batches; ok {


Was it intended to change the for by if?

Yes, otherwise either test or static check would fail. The updated code is equivalent to the previous one.

songy23 · 2019-08-02T16:07:00Z

Should be ready to merge now.

* Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update exporters README (#210) Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter. * Added Jaeger gRPC receiver (#197) * Added Jaeger gRPC receiver Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Addressed first review round Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Changes based on the feedback from the second review Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Fixed import order, check for same ip:port on endpoints Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Reverted the port-check Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Fix README - remove broken links and commands (#211) Signed-off-by: Hui Kang <[email protected]> * Refactor opencensus exporter to make it easily extensible (#212) Refactored oc exporter code to make it easier to import in derived builds without copying all the code. Example derived exporter: https://github.com/owais/example-derived-oc-exporter - Moved internal/compression to root - Split opencensusexporter factory's `CreateTraceExporter` method into - `OCAgentOptions` and `CreateOCAgent` * Use sha256 instead of md5 in node batcher processor (#202) MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications. * Add goimports check and fix import order for all files (#209) * Add goimports check and fix import order for all files Updates #155. * Try specifying a version for goimports * Show the diff instead of files and fix import * Fix default in Makefile * Add factory and update config for tail sampling processor (#200) * Add factory and update config for tail sampling processor Updates #146 * Move to package processor * Remove an unnecessary check * Move CreateDefaultConfig to factory and add unit tests * Fix test failure * Remove commented code * Add misspell check and fix all typos (#214) * Add misspell check and fix all typos Updates #155. * Format * Include yaml files * Move tail sampling config to its own package and remove stale configs (#217) Follow up of #200. Second step of #146. * Add Observability Vision document (#188) * Add Observability Vision document This is a guidance document that developers can consult with when working on improving own observability of OpenTelemetry Service. * PR fixes * Add Zipkin exporter factory (#207) * Add Zipkin exporter factory Add the Zipkin exporter configuration factory using the new configuration format. This does not bring many of the settings from the old configuration since they won't be used. In a sub-sequent PR the code of the exporter itself will be changed. * PR Feedback: add 1 test plus some comments * Rename endpoint to http-url and related field * Fix goimports issue * Remove TODO commented code Replaced the TODO commented code with an issue. * Rename the field used to specify destination * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Improve OC Receiver Documentation (#203) * First round of receiver and opencensus receiver documentation. * Undo go mod/sum changes * Address initial set of comments. * Address next set of comments. * Address next set of comments. * Fix use of server instead of receiver in comment and explain settings can be mix and matched. * Merged master and fixed mispell error caugh with new tools * Add static check and fix all errors (#218) * Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher * Rename exporterhelp parameter (#220) The paramater was named exportFormat but it actually used as the exporter name. * Fix build: lower casing error message (#224) * Add jaeger grpc exporter (#219) * Add Jaeger gRPC exporter Adds a Jaeger gRPC exporter. * Update exporters/README.md * Fixes on the exporter/README.md * Add doc.go with package description. * Fix import order on config_test.go * Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Fix goimports * Fix staticcheck issues

* Add SignalFx forwarder to default configs * Use layer caching on fpm builds * Document signalfx-forwarder as metric and trace receiver

songy23 requested review from bogdandrutu, flands, pjanotti and tigrannajaryan as code owners August 1, 2019 00:16

pjanotti reviewed Aug 1, 2019

View reviewed changes

songy23 force-pushed the static-check branch from 12a36ee to 090d80a Compare August 1, 2019 18:13

pjanotti reviewed Aug 1, 2019

View reviewed changes

songy23 added 2 commits August 1, 2019 11:31

Add static check

cae899f

Fixes open-telemetry#155.

Fix most staticcheck errors

e211122

songy23 force-pushed the static-check branch from 090d80a to e211122 Compare August 1, 2019 18:31

More fixes

33b223a

songy23 changed the title ~~[WIP] Add static check~~ Add static check and fix all errors Aug 1, 2019

songy23 self-assigned this Aug 1, 2019

Fix id_batcher

28325e4

pjanotti approved these changes Aug 1, 2019

View reviewed changes

tigrannajaryan reviewed Aug 1, 2019

View reviewed changes

tigrannajaryan approved these changes Aug 1, 2019

View reviewed changes

pjanotti merged commit 79957ff into open-telemetry:master Aug 2, 2019

songy23 deleted the static-check branch August 2, 2019 16:17

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

Fix up docs and other othttp cleanups (open-telemetry#218)

4172bdf

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Add OStream metrics exporter (open-telemetry#218)

6631930

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static check and fix all errors #218

Add static check and fix all errors #218

songy23 commented Aug 1, 2019

songy23 commented Aug 1, 2019

songy23 commented Aug 1, 2019

rghetia commented Aug 1, 2019

pjanotti commented Aug 1, 2019

songy23 commented Aug 1, 2019

pjanotti commented Aug 1, 2019

pjanotti left a comment

pjanotti left a comment

songy23 commented Aug 1, 2019

codecov-io commented Aug 1, 2019

pjanotti left a comment

tigrannajaryan Aug 1, 2019

songy23 Aug 1, 2019

songy23 commented Aug 2, 2019

Add static check and fix all errors #218

Add static check and fix all errors #218

Conversation

songy23 commented Aug 1, 2019

songy23 commented Aug 1, 2019

songy23 commented Aug 1, 2019

rghetia commented Aug 1, 2019

pjanotti commented Aug 1, 2019

songy23 commented Aug 1, 2019

pjanotti commented Aug 1, 2019

pjanotti left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

songy23 commented Aug 1, 2019

codecov-io commented Aug 1, 2019

Codecov Report

pjanotti left a comment

Choose a reason for hiding this comment

tigrannajaryan Aug 1, 2019

Choose a reason for hiding this comment

songy23 Aug 1, 2019

Choose a reason for hiding this comment

songy23 commented Aug 2, 2019