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

feat(performance): start transaction for echo middleware/integration #722

Merged
merged 10 commits into from
Mar 26, 2024

Conversation

aldy505
Copy link
Contributor

@aldy505 aldy505 commented Oct 1, 2023

Closes #642

Let me know if I should also edit CHANGELOG.md too.

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.76%. Comparing base (3c523e2) to head (988249a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #722      +/-   ##
==========================================
+ Coverage   81.59%   81.76%   +0.16%     
==========================================
  Files          48       48              
  Lines        4717     4759      +42     
==========================================
+ Hits         3849     3891      +42     
  Misses        728      728              
  Partials      140      140              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zKoz210
Copy link

zKoz210 commented Dec 27, 2023

@aldy505 is there any news regarding the implementation? I could really use this

@aldy505
Copy link
Contributor Author

aldy505 commented Dec 27, 2023

@aldy505 is there any news regarding the implementation? I could really use this

Michi (@cleptric) said to me a few weeks (or month?) ago over Discord that the Go SDK would need to prioritize to convert every performance section to use OpenTelemetry under the hood first before any other changes. That.. would mean this PR is on hold until the performance section is refactored.

Now, for you, would obviously not a good idea to wait for this PR to be merged. But one thing you can do is to copy and paste everything inside the echo directory (exclude the test, it's fine) to your project as your own "sentryecho" package. Then import your own "sentryecho" package instead of this one. I think that is doable on your part.

@aldy505
Copy link
Contributor Author

aldy505 commented Feb 27, 2024

Since the perf by otel is on halt, here's a gentle ping @cleptric

echo/example_test.go Outdated Show resolved Hide resolved
echo/sentryecho.go Outdated Show resolved Hide resolved
echo/sentryecho.go Outdated Show resolved Hide resolved
@aldy505 aldy505 requested a review from cleptric March 2, 2024 00:25
@aldy505
Copy link
Contributor Author

aldy505 commented Mar 2, 2024

I wanted to add starfish properties (or otel properties, really) as seen in #786, but I think it's better for this and #723 to be merged first, and I update #786 to include both echo and fasthttp. Or.. it can be a separate PR.

@cleptric cleptric requested a review from ribice March 14, 2024 11:38
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Any chance you could add a changelog entry?

@aldy505
Copy link
Contributor Author

aldy505 commented Mar 26, 2024

LGTM, thanks.

Any chance you could add a changelog entry?

@vaind in 3-4 hours from now

@vaind
Copy link
Collaborator

vaind commented Mar 26, 2024

LGTM, thanks.
Any chance you could add a changelog entry?

@vaind in 3-4 hours from now

I've noticed this repo doesn't have rules to prepare changelog in advance with PRs. Added #799 to fix that, afterwards we can add the entry in this PR.

@vaind vaind enabled auto-merge (squash) March 26, 2024 13:03
@vaind vaind merged commit 5942155 into getsentry:master Mar 26, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to send performance data to Sentry (echo integration)
5 participants