-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port servicegraphprocessor to servicegraphconnector #18389
Port servicegraphprocessor to servicegraphconnector #18389
Conversation
Foresight Summary
View More Details✅ Test Kubernetes Related Components workflow has finished in 16 minutes 59 seconds and finished at 6th Feb, 2023.
⭕ build-and-test-windows workflow has finished in 3 seconds (40 minutes 46 seconds less than
|
Job | Failed Steps | Tests | |
---|---|---|---|
windows-unittest-matrix | - 🔗 | N/A | See Details |
windows-unittest | - 🔗 | N/A | See Details |
✅ telemetrygen workflow has finished in 1 minute (1 minute 55 seconds less than main
branch avg.) and finished at 10th Feb, 2023.
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 3 minutes 12 seconds (⚠️ 44 seconds more than main
branch avg.) and finished at 10th Feb, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
changed files | - 🔗 | N/A | See Details |
check-links | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 4 minutes 19 seconds and finished at 10th Feb, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
changelog | - 🔗 | N/A | See Details |
✅ prometheus-compliance-tests workflow has finished in 11 minutes 58 seconds (⚠️ 3 minutes 25 seconds more than main
branch avg.) and finished at 10th Feb, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
prometheus-compliance-tests | - 🔗 | ✅ 21 ❌ 0 ⏭ 0 🔗 | See Details |
✅ e2e-tests workflow has finished in 15 minutes 22 seconds (3 minutes 20 seconds less than main
branch avg.) and finished at 10th Feb, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
kubernetes-test | - 🔗 | N/A | See Details |
❌ load-tests workflow has finished in 28 minutes 22 seconds (⚠️ 11 minutes 56 seconds more than main
branch avg.) and finished at 10th Feb, 2023. 1 job failed. There are 2 test failures.
Job | Failed Steps | Tests | |
---|---|---|---|
loadtest (TestIdleMode) | - 🔗 | ✅ 1 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestMetric10kDPS|TestMetricsFromFile) | - 🔗 | ✅ 6 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestTraceAttributesProcessor) | - 🔗 | ✅ 3 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) | - 🔗 | ✅ 8 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) | - 🔗 | ✅ 10 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) | - 🔗 | ✅ 12 ❌ 0 ⏭ 0 🔗 | See Details |
loadtest (TestBallastMemory|TestLog10kDPS) | Loadtest 🔗 | ✅ 17 ❌ 2 ⏭ 0 🔗 | See Details |
setup-environment | - 🔗 | N/A | See Details |
✅ build-and-test workflow has finished in 58 minutes 19 seconds and finished at 10th Feb, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
correctness-metrics | - 🔗 | ✅ 2 ❌ 0 ⏭ 0 🔗 | See Details |
correctness-traces | - 🔗 | ✅ 17 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, processor) | - 🔗 | ✅ 1479 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, processor) | - 🔗 | ✅ 1479 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, exporter) | - 🔗 | ✅ 2452 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, exporter) | - 🔗 | ✅ 2452 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, receiver-1) | - 🔗 | ✅ 1959 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, receiver-1) | - 🔗 | ✅ 1959 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.18, other) | - 🔗 | ✅ 4675 ❌ 0 ⏭ 0 🔗 | See Details |
integration-tests | - 🔗 | ✅ 55 ❌ 0 ⏭ 0 🔗 | See Details |
unittest-matrix (1.19, other) | - 🔗 | ✅ 4675 ❌ 0 ⏭ 0 🔗 | See Details |
setup-environment | - 🔗 | N/A | See Details |
lint-matrix (receiver-0) | - 🔗 | N/A | See Details |
lint-matrix (receiver-1) | - 🔗 | N/A | See Details |
lint-matrix (processor) | - 🔗 | N/A | See Details |
lint-matrix (exporter) | - 🔗 | N/A | See Details |
lint-matrix (internal) | - 🔗 | N/A | See Details |
build-examples | - 🔗 | N/A | See Details |
checks | - 🔗 | N/A | See Details |
check-collector-module-version | - 🔗 | N/A | See Details |
lint-matrix (extension) | - 🔗 | N/A | See Details |
lint-matrix (other) | - 🔗 | N/A | See Details |
check-codeowners | - 🔗 | N/A | See Details |
lint | - 🔗 | N/A | See Details |
unittest (1.19) | - 🔗 | N/A | See Details |
unittest (1.18) | - 🔗 | 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-stable | - 🔗 | N/A | See Details |
publish-dev | - 🔗 | N/A | See Details |
*You can configure Foresight comments in your organization settings page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. TIL about connectors, and sound perfect for these types of components.
This PR will result in duplicated code bases, what are the following steps after this is merged? The processor gets marked as deprecated? I'd prefer to have both component at the same time for as short as possible.
What I have talked to @kovrus (and perhaps @djaglowski, not sure), is to have both versions available for some time, allowing for a generous grace period for users to migrate over. The config migration should be easy, but not effortless: they are different component types even though the component's config is identical. We have not identified reasons for metrics to not be backward compatible, so that part should be covered. |
I agree with @jpkrohling's suggested process, with one caveat. As excited as I am about the connectors framework, I think we should exercise caution in the short term. The ability to use connectors will be included in this week's release, but will be disabled by default, behind an I think we can merge this whenever we are comfortable with the implementation but that we should not deprecate the processor and encourage migration until the connectors framework is enabled by default ( In other cases, I would suggest even more caution before recommending a migration. However, this processor is still |
What I'm worried about are both code bases diverging—eg. a contribution is being worked on in the processor (PR), that as soon as its merged will already make this PR outdated. From what I've checked |
@mapno, seems reasonable. I'll try reuse what can be reused in the processor. |
88735b5
to
00783ac
Compare
ccee8d1
to
60743fb
Compare
I reworked this so that the entire implementation remains in the processor. This seems ideal because we do not need to export anything additional. The code ends up with some |
3699079
to
387b670
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
minor, some of the logs contain the servicegraphprocessor
name which might be confusing for connector users.
25e175a
to
e46b31e
Compare
I updated the log messages to account for the component type |
I think this should be good to go, unless there is more feedback. @jpkrohling, do you want to take a look? |
e46b31e
to
da122f6
Compare
Similar to #18381. This is a copy/paste/update that results in a simpler codebase and config.
Still waiting for the framework to be merged into contrib before this can be fully validated, but it seemed like a pretty clean conversion.