For Code Review: All files #8

dabangarang · 2020-08-05T08:37:26Z

To Yang: I'm PRing all files, but if you feel as though we need to go over Config or Factory (Config is done, if I'm not mistaken) let me know before the meeting.

…for service package

…ollector into unit-tests

exporter/cortexexporter/cortex.go

PettitWesley · 2020-08-05T21:11:12Z

exporter/cortexexporter/cortex.go

+	}
+
+	//Only even status codes < 400 are okay
+	if httpResp.StatusCode/100 != 2 || httpResp.StatusCode >= 400 {


This condition feels unnecessarily weird... why not do something more readable:

httpResp.StatusCode != 200 || ..

HTTP Status codes 2XX are generally considered successful. "Only even status codes" in the comment is not explicitly clear what you mean and not a safe assumption. What Status Code(s) do we expect to be returned from the call to ce.client.Do(httpReq) above?

We are expecting a 2XX status code. What we were thinking is distinguishing whether an error is recoverable or not, and let the collector initiate retry based on that. We were confused on the status code and will update it to checking for 2XX.

I'd look to narrow it down to handling specific status codes we expect to be returned by the API we're interfacing with. Services don't generally return such a broad range (all 2XX for example) of status codes unless you're interfacing with multiple endpoints and we'd generally want to know if the success status code being returned changed as it might indicate changes in functionality or something else. Those types of changes/issues would fly under the radar if we're accepting all 2XX status codes.

Great suggestion, thanks! I think a reason we were hesitant to specify what code specifically is because theoretically the exporter could send to Cortex and other different remote write API integrated backends, and we aren't sure what the exact behavior of each backend is. We will look for a single spec and update this part.

I looked at Cortex code and it logs an error on its side if the response code is not 202 Accepted. We will change the code to reflect the same behavior as well. If that becomes a problem for other backends, we would change it back.

PettitWesley · 2020-08-05T21:23:22Z

exporter/cortexexporter/cortex.go

+	//Add necessary headers: https://cortexmetrics.io/docs/apis/#remote-api
+	httpReq.Header.Add("Content-Encoding", "snappy")
+	httpReq.Header.Set("Content-Type", "application/x-protobuf")
+	httpReq.Header.Set("X-Prometheus-Remote-Write-Version", "0.1.0")


Should this be hard coded?

As of right now, it seems as though these currrent parameter headers are necessary to interact with the Corte gateway: https://cortexmetrics.io/docs/apis/#remote-api
Would you recommend us trying to dynamically change this to new versions as they come out-- if they come out?

Merge branch 'all_files_v1' of https://github.com/open-o11y/opentelemetry-collector into all_files_v1

github-actions · 2020-09-16T03:21:24Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2020-09-23T03:21:57Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

* Initial commit * Add CODEOWNERS file (#2) * Add CODEOWNERS file * Update CODEOWNERS * Moved from github.com/observatorium/opentelemetry-collector-builder (#3) Signed-off-by: Juraci Paixão Kröhling <[email protected]> * fixed panics (#6) Signed-off-by: Joe Elliott <[email protected]> * Replace master with main in CI and mergify files (#8) Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Bump to OpenTelemetry Collector 0.20.0 (#10) Closes #9 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Explicitly enable Go modules in quickstart instructions (#13) * Update to collector v0.21.0 (#17) Fixes #16 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Update to collector v0.22.0 (#19) * Download go modules before building (#20) Fixes #14 * Add version command (#25) Signed-off-by: Ashmita Bohara <[email protected]> * Pass errors from cobra Execute back to main for correct exit code (#28) * pass errors from cobra execute back to main * print the error * Update to collector v0.23.0 (#27) * Generate a warning if the builder and collector base version mismatch (#30) * Generate a warning if the builder and collector base version mismatch * Show current default version in the warning message * Update to OpenTelemetry Collector 0.24.0 * Don't use %w formatting with log.Fatal (#35) * Update to OpenTelemetry Collector 0.25.0 (#36) Signed-off-by: Serge Catudal <[email protected]> * Update to 0.26.0 and update BuildInfo (#39) * Sync build and CI Go versions at latest 1.16 (#34) * Sync build and CI Go versions at latest 1.16 * Run go mod tidy * Set go binary to use in the compilation phase in tests Signed-off-by: Juraci Paixão Kröhling <[email protected]> Co-authored-by: Juraci Paixão Kröhling <[email protected]> * Add option to generate go code only (no compile) (#40) * Issue#24 Add option to generate go code only (no compile) * Update cmd/root.go logging Suggested by @jpkkrohling Co-authored-by: Juraci Paixão Kröhling <[email protected]> * remove verbose help .. created by corba * suggestion by jpkrohling to keep generateandcompile * lint error: remove unused var * reword cmd option and add back help message for default Co-authored-by: Juraci Paixão Kröhling <[email protected]> * Don't reuse exec.Cmd (#42) * Update to OpenTelemetry Collector 0.27.0 (#43) * Add CI Badge (#47) * Update to Collector v0.28.0 (#49) * Update to Collector v0.28.0 Closes #48 Addresses the breaking API change in open-telemetry#3163, besides the usual version number changes. Signed-off-by: Fangyi Zhou <[email protected]> * Use `go mod tidy` instead of `go mod download` It appears that this magically resolves the go.mod file issue. https://stackoverflow.com/questions/67203641/missing-go-sum-entry-for-module-providing-package-package-name Signed-off-by: Fangyi Zhou <[email protected]> * Account for go mod download in go1.17 not updating go.sum (#50) * Update to collector v0.29.0 (#54) * Update replaces.builder.yaml * Update nocore.builder.yaml * Update config.go * Update README.md * Update main.go * Update to collector v0.30.0 (#57) * cmd: fix module flag default value to github.com/open-telemetry (#58) Signed-off-by: Koichi Shiraishi <[email protected]> * Update to collector v0.31.0 (#60) * Update to v0.33.0 (#62) Signed-off-by: Anthony J Mirabella <[email protected]> * Add excludes support to generated go.mod (#63) Signed-off-by: Anthony J Mirabella <[email protected]> Co-authored-by: Juraci Paixão Kröhling <[email protected]> * Small cleanup for the builder files (#64) Signed-off-by: Bogdan Drutu <[email protected]> * Support building with Go 1.17 (#66) * Support building with Go 1.17 Fixes #65 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Update workflows to use Go 1.17 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Add gosec exceptions for exec.Command Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Update to OpenTelemetry core 0.34.0 (#68) Fixes #67 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Upgrade to OpenTelemetry Collector 0.35.0 (#70) Signed-off-by: Fangyi Zhou <[email protected]> * Upgrade to OpenTelemetry Collector 0.36.0 (#76) * Generate custom service code for Windows (#75) * update main to include windows service code * use main version from tag 0.35.0 * update main function * align with upstream v0.36.0 tag * dummy change to trigger build * Revert "dummy change to trigger build" This reverts commit 629d499461da2d2c240bf1e495b5fe0558e3547f. * Remove Core from Module type (#77) Fixes #15 Signed-off-by: yugo-horie <[email protected]> * release 0.37.0 (#78) * release 0.37.0 * update use of NewCommand * Move builder to subdirectory Signed-off-by: Juraci Paixão Kröhling <[email protected]> Co-authored-by: Bogdan Drutu <[email protected]> Co-authored-by: Bogdan Drutu <[email protected]> Co-authored-by: Joe Elliott <[email protected]> Co-authored-by: Eric Yang <[email protected]> Co-authored-by: Brian Gibbins <[email protected]> Co-authored-by: Ashmita <[email protected]> Co-authored-by: Fangyi Zhou <[email protected]> Co-authored-by: Shaun Creary <[email protected]> Co-authored-by: Patryk Małek <[email protected]> Co-authored-by: Serge Catudal <[email protected]> Co-authored-by: Aaron Stone <[email protected]> Co-authored-by: Patryk Małek <[email protected]> Co-authored-by: Aaron Stone <[email protected]> Co-authored-by: Kelvin Lo <[email protected]> Co-authored-by: Himanshu <[email protected]> Co-authored-by: Y.Horie <[email protected]> Co-authored-by: Koichi Shiraishi <[email protected]> Co-authored-by: Anthony Mirabella <[email protected]> Co-authored-by: Cal Loomis <[email protected]> Co-authored-by: alrex <[email protected]>

huyan0 and others added 30 commits July 29, 2020 22:45

set up project structure

06753d6

placeholder text for README

287a4e5

finish Test_validateMetrics

c7e2a42

finish Test_validateMetrics

0f6033f

finish Test_validateMetrics

0686608

remove extra import

a896535

fix format error so that make runs

6335403

update unit tests

f76b22a

Testbed for the config and factory constructions:

91dddb9

Testbed for the config and factory constructions, possible README.md …

66224aa

…for service package

Testbed oop

2e4b1e1

add metrictest util code

01441e7

tempeorary code

a9eaaec

finish testutil and Test_addSample

7cf119b

add more unit tests

5625330

add unit tests

cbd7f6e

use assert.Exactly

a4d7eeb

finish Test_handleHistogramMetric

ae1758c

fix Test_handleScalarMetric to include metric name in label set

93df3e9

finish Test_handleSummaryMetric

346fce0

add Test_shutdown

d215b33

add unit test for functions

b001c18

add unit tests for cortex.go

64aad65

associated methods with cortexExporter

b55576e

some more config and factory stuff:

ad4f1b1

M branch 'unit-tests' of https://github.com/open-o11y/opentelemetry-c…

5806a51

…ollector into unit-tests

config and some factory, export skeleton

d670578

config and some factory, export skeleton

d125a28

add nil test cases for cortex.go

34b40fe

implement and test validateMetrics and addSample

32a9db9

huyan0 added 2 commits August 5, 2020 15:42

add extra line

ef86ae4

add blank line at the end of the file

cb0009b

PettitWesley reviewed Aug 5, 2020

View reviewed changes

exporter/cortexexporter/cortex.go Outdated Show resolved Hide resolved

PettitWesley reviewed Aug 5, 2020

View reviewed changes

exporter/cortexexporter/cortex.go Outdated Show resolved Hide resolved

PettitWesley reviewed Aug 5, 2020

View reviewed changes

huyan0 and others added 17 commits August 5, 2020 19:34

fix merge conflict

b0135df

update README.md

6059ee6

rename testutil.go to testutil_test.go

4ae8d41

address Wesley's comments

4867718

address Eric's comments

b9a56a1

address EdZou's comments

e173d35

add histogram test to Test_pushMetrics

7db4e71

rename lbls to labels

cfeefe6

improve code coverage and build success

0a72c51

improve code coverage and build success

9ca9300

3am reformatting, very minor, more to come tomorrow before the PR hehe

7b41ecd

rename to prometheus remote write exporter

7e66b32

change name to Prometheus remote write

5f7b486

update README.md

71ded56

another round of lookovers

95938e8

add comments

f0ddcae

Merge branch 'all_files_v1' of https://github.com/open-o11y/opentelemetry-collector into all_files_v1

yay

16553e0

connorlindsey approved these changes Aug 7, 2020

View reviewed changes

github-actions bot added the Stale label Sep 16, 2020

github-actions bot closed this Sep 23, 2020

JamesJHPark pushed a commit that referenced this pull request Nov 3, 2021

Replace master with main in CI and mergify files (#8)

40a858e

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For Code Review: All files #8

For Code Review: All files #8

dabangarang commented Aug 5, 2020

PettitWesley Aug 5, 2020

sonofachamp Aug 5, 2020

huyan0 Aug 6, 2020

sonofachamp Aug 7, 2020

huyan0 Aug 7, 2020 •

edited

Loading

huyan0 Aug 7, 2020 •

edited

Loading

PettitWesley Aug 5, 2020

dabangarang Aug 7, 2020

github-actions bot commented Sep 16, 2020

github-actions bot commented Sep 23, 2020

For Code Review: All files #8

For Code Review: All files #8

Conversation

dabangarang commented Aug 5, 2020

PettitWesley Aug 5, 2020

Choose a reason for hiding this comment

sonofachamp Aug 5, 2020

Choose a reason for hiding this comment

huyan0 Aug 6, 2020

Choose a reason for hiding this comment

sonofachamp Aug 7, 2020

Choose a reason for hiding this comment

huyan0 Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

huyan0 Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

PettitWesley Aug 5, 2020

Choose a reason for hiding this comment

dabangarang Aug 7, 2020

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2020

github-actions bot commented Sep 23, 2020

huyan0 Aug 7, 2020 •

edited

Loading

huyan0 Aug 7, 2020 •

edited

Loading