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

[collector] use spanmetrics connector instead of spanmetrics processor #829

Merged

Conversation

fatsheep9146
Copy link
Contributor

@fatsheep9146 fatsheep9146 commented Apr 9, 2023

Changes

I would suggest updating opentelemetry-demo to use the spanmetrics connector instead of the spanmetrics processor, as the latter has been deprecated.

By doing this we can also let user to take advantage of the new advanced functionality connector.

I also modify the spanmetric dashboard to be compatible with breaking changes from the spanmetrics connector, which are listed in (here)[Compatibility with important changes to the Spanmetrics connector
]

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs

@fatsheep9146 fatsheep9146 requested a review from a team as a code owner April 9, 2023 04:47
@julianocosta89
Copy link
Member

Assuming this is outdated: https://github.com/open-telemetry/opentelemetry-collector/blob/main/connector/README.md#status
It looks good to me.

If the connectors are still in alpha, than, I'd rather wait to it to get to at least to beta. Otherwise, maintaining all the possible changes will be a pain.

@austinlparker, you have mentioned that the README was probably outdated, right?

@julianocosta89
Copy link
Member

@fatsheep9146 could you also add a changelog?

@fatsheep9146
Copy link
Contributor Author

@fatsheep9146 could you also add a changelog?

Done

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
@fatsheep9146
Copy link
Contributor Author

If the connectors are still in alpha, than, I'd rather wait to it to get to at least to beta. Otherwise, maintaining all the possible changes will be a pain.

I think this also makes sense. But I just want to experience with this useful feature, since the connector and spanmetrics connector are both marvelous features introduced into collector. And I think their design is really fully reviewed.
If you think we should not use them until they enter into beta, I'm also ok with that. :)

@julianocosta89

@julianocosta89
Copy link
Member

I had some personal issues with beta releases b4 😅 .
But that's a me thing.

I'm good to go either way, just want to hear from everyone first.

@austinlparker
Copy link
Member

Given the cadence of collector releases and how quickly things move thru maturity phases, I'd argue that we should pick up connectors now in order to get useful feedback for collector devs?

@austinlparker austinlparker merged commit aec77cf into open-telemetry:main Apr 10, 2023
@puckpuck puckpuck added helm-update-required Requires an update to the Helm chart when released v1.4 required for 1.4 release labels Apr 11, 2023
juliangiuca pushed a commit to juliangiuca/opentelemetry-demo that referenced this pull request Apr 12, 2023
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released v1.4 required for 1.4 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants