Skip to content
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

Make component interfaces uniform #488

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Jan 3, 2020

This change fixes inconsistencies in component interfaces. Motivation:

  • Uniformness results in reduction of code that currently has to deal with differences.
  • Processor.Start is missing and is important for allowing processors to communicate with the Host.

What's changed:

  • Introduced Component interface.
  • Unified Host interface.
  • Added a Start function to processors (via Component interface).
  • Receivers, Exporters, Processors, Extensions now embed Component interface.
  • Replaced StartTraceReception/StartMetricsReception by single Start function for receivers.
  • Replaced StopTraceReception/StopMetricsReception by single Shutdown function for receivers.

Note: before merging this we need to announce the change in Gitter since it breaks existing implementations in contrib (although the fix is easy).

Resolves #477
Resolves #262

@codecov-io
Copy link

codecov-io commented Jan 3, 2020

Codecov Report

Merging #488 into master will decrease coverage by 0.01%.
The diff coverage is 52.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
- Coverage   75.78%   75.76%   -0.02%     
==========================================
  Files         120      120              
  Lines        7391     7431      +40     
==========================================
+ Hits         5601     5630      +29     
- Misses       1523     1534      +11     
  Partials      267      267
Impacted Files Coverage Δ
receiver/factory.go 100% <ø> (ø) ⬆️
...er/prometheusreceiver/internal/metrics_adjuster.go 92.81% <ø> (ø) ⬆️
processor/batchprocessor/node_batcher.go 90.13% <0%> (-1.21%) ⬇️
...babilisticsamplerprocessor/probabilisticsampler.go 94.11% <0%> (-1.89%) ⬇️
exporter/exporterhelper/tracehelper.go 95.45% <0%> (ø) ⬆️
processor/queuedprocessor/queued_processor.go 45.75% <0%> (-0.61%) ⬇️
processor/spanprocessor/span.go 85.71% <0%> (-4.29%) ⬇️
processor/attributesprocessor/attributes.go 93.25% <0%> (-2.15%) ⬇️
exporter/exportertest/sink_exporter.go 69.23% <0%> (ø) ⬆️
exporter/exportertest/nop_exporter.go 85.18% <0%> (ø) ⬆️
... and 22 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 60b03d0...7399146. Read the comment docs.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/unified-component branch from ff0d6d2 to e9e0317 Compare January 6, 2020 18:58
@tigrannajaryan tigrannajaryan changed the title [WIP] [DO NOT MERGE] Make component interfaces uniform [DO NOT MERGE YET] Make component interfaces uniform Jan 6, 2020
@tigrannajaryan tigrannajaryan removed the WIP label Jan 6, 2020
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/unified-component branch from e9e0317 to dc8d621 Compare January 6, 2020 20:54
@tigrannajaryan
Copy link
Member Author

@open-telemetry/collector-maintainers please review.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

Probably not worth to do in the same PR but have you considered having a CreateReceiver factory method and using type assertion to check if a given receiver supports TraceReceiver and/or MetricsReceiver interfaces? I didn't think about all implications of this, but, it seems a path worth exploring.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/unified-component branch from dc8d621 to 4f02aeb Compare January 8, 2020 16:00
This change fixes inconsistencies in component interfaces. Motivation:

- Uniformness results in reduction of code that currently has to
  deal with differences.
- Processor.Start is missing and is important for allowing processors
  to communicate with the Host.

What's changed:

- Introduced Component interface.
- Unified Host interface.
- Added a Start function to processors (via Component interface).
- Start/Shutdown is now called for Processors from service start/shutdown.
- Receivers, Exporters, Processors, Extensions now embed Component interface.
- Replaced StartTraceReception/StartMetricsReception by single Start function for receivers.
- Replaced StopTraceReception/StopMetricsReception by single Shutdown function for receivers.

Note: before merging this we need to announce the change in Gitter since it
breaks existing implementations in contrib (although the fix is easy).

Resolves open-telemetry#477
Resolves open-telemetry#262
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/unified-component branch from 4f02aeb to 7399146 Compare January 8, 2020 17:24
@tigrannajaryan
Copy link
Member Author

Probably not worth to do in the same PR but have you considered having a CreateReceiver factory method and using type assertion to check if a given receiver supports TraceReceiver and/or MetricsReceiver interfaces? I didn't think about all implications of this, but, it seems a path worth exploring.

@pjanotti I didn't think about this. Can explore in the future.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tigrannajaryan tigrannajaryan changed the title [DO NOT MERGE YET] Make component interfaces uniform Make component interfaces uniform Jan 10, 2020
@tigrannajaryan
Copy link
Member Author

I am going to merge this. The change was announced in gitter. After merging this I will update the contrib repo.

@tigrannajaryan tigrannajaryan merged commit 91728bc into open-telemetry:master Jan 10, 2020
@tigrannajaryan tigrannajaryan deleted the feature/tigran/unified-component branch January 10, 2020 13:49
subnova pushed a commit to subnova/opentelemetry-collector that referenced this pull request Jan 14, 2020
This change fixes inconsistencies in component interfaces. Motivation:

- Uniformness results in reduction of code that currently has to
  deal with differences.
- Processor.Start is missing and is important for allowing processors
  to communicate with the Host.

What's changed:

- Introduced Component interface.
- Unified Host interface.
- Added a Start function to processors (via Component interface).
- Start/Shutdown is now called for Processors from service start/shutdown.
- Receivers, Exporters, Processors, Extensions now embed Component interface.
- Replaced StartTraceReception/StartMetricsReception by single Start function for receivers.
- Replaced StopTraceReception/StopMetricsReception by single Shutdown function for receivers.

Note: before merging this we need to announce the change in Gitter since it
breaks existing implementations in contrib (although the fix is easy).

Resolves open-telemetry#477
Resolves open-telemetry#262
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
* Update demo chart codeowners

* Bump demo chart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make component interfaces uniform Rename Stop*Receiver to Shutdown*Receiver
4 participants