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

Revert EndUserSpanProcessor integration #39648

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

brunobat
Copy link
Contributor

@brunobat brunobat commented Mar 22, 2024

Remove the functionality due to reliability issues.
Mitigates ##39563
The feature will be later re-implemented.

Original implementation: #34595

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 22, 2024

/cc @radcortez (opentelemetry)

@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Mar 22, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 22, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit f76d812.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

github-actions bot commented Mar 22, 2024

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

Quarkus Documentation automation moved this from To do to Reviewer approved Mar 25, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 25, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit f76d812.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gsmet gsmet merged commit 457a025 into quarkusio:main Mar 25, 2024
30 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Mar 25, 2024
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 25, 2024
@gsmet
Copy link
Member

gsmet commented Mar 25, 2024

I merged it but I'm not entirely sure how we should handle it in currently maintained branches.

Should we at least backport the removal of the documentation and mark the config as deprecated so that people don't see it in the doc?

@michalvavrik
Copy link
Contributor

+1 for removal via backports because to fix this there would require a lot of work. Fixing it for main is doable.

@gsmet
Copy link
Member

gsmet commented Mar 25, 2024

Yeah problem is that removing a feature with a backport is definitely too aggressive IMO.

I would:

  • Add a warning saying it shouldn't be used
  • Remove it from the doc
  • Make sure the config doesn't appear in the config reference

That means except if using it could lead to serious issues. In this case, yes, we could discuss dropping it. But we would need a broader discussion than here in the comment section of a PR.

@michalvavrik
Copy link
Contributor

That means except if using it could lead to serious issues. In this case, yes, we could discuss dropping it.

My bit: Risks are ugly warning, unreliable feature. I think raised exceptions won't have recognizable impact on resources.

I think @brunobat will have a plan what to do.

@brunobat
Copy link
Contributor Author

I agree on creating a backport PR including:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants