receiver_creator: don't embed endpoint field in unsupported configs #18641
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
Foresight Summary
View More Details✅ telemetrygen workflow has finished in 1 minute 8 seconds (1 minute 57 seconds less than
|
| Job | Failed Steps | Tests | |
|---|---|---|---|
| build-dev | - 🔗 | N/A | See Details |
| publish-latest | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
✅ check-links workflow has finished in 1 minute 28 seconds (1 minute 5 seconds less than main branch avg.) and finished at 15th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changed files | - 🔗 | N/A | See Details |
| check-links | - 🔗 | N/A | See Details |
✅ build-and-test workflow has finished in 42 minutes 23 seconds (23 minutes 9 seconds less than main branch avg.) and finished at 15th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| unittest-matrix (1.19, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, processor) | - 🔗 | ✅ 1528 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, processor) | - 🔗 | ✅ 1528 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-metrics | - 🔗 | ✅ 2 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-traces | - 🔗 | ✅ 17 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, exporter) | - 🔗 | ✅ 2455 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, exporter) | - 🔗 | ✅ 2455 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, receiver-1) | - 🔗 | ✅ 1933 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, other) | - 🔗 | ✅ 4687 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-1) | - 🔗 | ✅ 1933 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, other) | - 🔗 | ✅ 4687 ❌ 0 ⏭ 0 🔗 | See Details |
| integration-tests | - 🔗 | ✅ 55 ❌ 0 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
| lint-matrix (receiver-1) | - 🔗 | N/A | See Details |
| lint-matrix (processor) | - 🔗 | N/A | See Details |
| lint-matrix (receiver-0) | - 🔗 | N/A | See Details |
| lint-matrix (extension) | - 🔗 | N/A | See Details |
| lint-matrix (exporter) | - 🔗 | N/A | See Details |
| checks | - 🔗 | N/A | See Details |
| build-examples | - 🔗 | N/A | See Details |
| lint-matrix (internal) | - 🔗 | N/A | See Details |
| check-collector-module-version | - 🔗 | N/A | See Details |
| lint-matrix (other) | - 🔗 | N/A | See Details |
| check-codeowners | - 🔗 | N/A | See Details |
| lint | - 🔗 | N/A | See Details |
| unittest (1.20) | - 🔗 | N/A | See Details |
| unittest (1.19) | - 🔗 | N/A | See Details |
| cross-compile (darwin, amd64) | - 🔗 | N/A | See Details |
| cross-compile (darwin, arm64) | - 🔗 | N/A | See Details |
| cross-compile (linux, 386) | - 🔗 | N/A | See Details |
| cross-compile (linux, amd64) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm64) | - 🔗 | N/A | See Details |
| cross-compile (linux, ppc64le) | - 🔗 | N/A | See Details |
| cross-compile (windows, 386) | - 🔗 | N/A | See Details |
| cross-compile (windows, amd64) | - 🔗 | N/A | See Details |
| build-package (deb) | - 🔗 | N/A | See Details |
| build-package (rpm) | - 🔗 | N/A | See Details |
| windows-msi | - 🔗 | N/A | See Details |
| publish-check | - 🔗 | N/A | See Details |
| publish-dev | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
✅ load-tests workflow has finished in 7 minutes 53 seconds (9 minutes 4 seconds less than main branch avg.) and finished at 15th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| loadtest (TestIdleMode) | - 🔗 | ✅ 1 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceAttributesProcessor) | - 🔗 | ✅ 3 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetric10kDPS|TestMetricsFromFile) | - 🔗 | ✅ 6 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) | - 🔗 | ✅ 8 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetricResourceProcessor|TestTrace10kSPS) | - 🔗 | ✅ 12 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) | - 🔗 | ✅ 10 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestBallastMemory|TestLog10kDPS) | - 🔗 | ✅ 18 ❌ 0 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
✅ prometheus-compliance-tests workflow has finished in 12 minutes 37 seconds (⚠️ 3 minutes 43 seconds more than main branch avg.) and finished at 15th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| prometheus-compliance-tests | - 🔗 | ✅ 21 ❌ 0 ⏭ 0 🔗 | See Details |
✅ e2e-tests workflow has finished in 12 minutes 50 seconds (3 minutes 35 seconds less than main branch avg.) and finished at 15th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| kubernetes-test | - 🔗 | N/A | See Details |
⭕ build-and-test-windows workflow has finished in 9 seconds (41 minutes 10 seconds less than main branch avg.) and finished at 25th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| windows-unittest-matrix | - 🔗 | N/A | See Details |
| windows-unittest | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 2 minutes 15 seconds and finished at 25th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changelog | - 🔗 | N/A | See Details |
*You can configure Foresight comments in your organization settings page.
rmfitzpatrick
commented
Feb 14, 2023
| @@ -57,6 +57,14 @@ func (mh *mockHostFactories) GetExtensions() map[component.ID]component.Componen | |||
| return mh.extensions | |||
| } | |||
|
|
|||
| var portRule = func(s string) rule { | |||
There was a problem hiding this comment.
The linter required this change since newRuleOrPanic() was always used with the same argument.
rmfitzpatrick
force-pushed
the
rcendpointfieldcheck
branch
from
2253ec1
to
62cb98d
Compare
atoulme
added
the
ready to merge
Comment on lines
+101
to
+102
| require.Error(t, mr.lastError) | ||
| require.EqualError(t, mr.lastError, test.expectedError) |
There was a problem hiding this comment.
Line 101 isn't needed since it is tested with 102.
| assert.Equal(t, 0, handler.receiversByEndpointID.Size()) | ||
| require.Error(t, mr.lastError) | ||
| require.EqualError(t, mr.lastError, test.expectedError) | ||
| require.Nil(t, mr.startedComponent) |
There was a problem hiding this comment.
I would leave this as assert instead of require since there is no risk of panicing and it is nice to know all errors within one test.
| require.NotNil(t, v) | ||
| actualConfig = v.cfg | ||
| default: | ||
| t.Fatalf("unexpected startedComponent: %T", v) |
There was a problem hiding this comment.
I believe there is require.Fail(...) that you can use here instead of calling t directly, helps keep the test output consistent.
| @@ -69,3 +71,59 @@ func Test_loadAndCreateRuntimeReceiver(t *testing.T) { | |||
| }()) | |||
| }) | |||
| } | |||
|
|
|||
| func TestValidateSetEndpointFromConfig(t *testing.T) { | |||
There was a problem hiding this comment.
I would consider breaking this test up into a test table so that you can use assert instead of require to help surface errors within the same test run instead of requiring incremental changes that could conflict with each other.
newly12
pushed a commit
to newly12/opentelemetry-collector-contrib
that referenced
this pull request
Feb 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description:
Fixing a bug - These changes add embedded config
endpointfield validation to the receiver creator so that it doesn't try to set the field for a config that doesn't support it. Also including a small refactor of the expansion logic to support more formats like those of the prometheus receiver.Link to tracking Issue:
#7381
Testing:
Updates existing observer handler tests to be more exhaustive than mock method call validation checks and adds more coverage for expansion suite.
Documentation:
Updated readme to describe the updated behavior.