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.

Sign up for GitHub

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

Jump to bottom

fix: Update span ops #655

Merged
merged 2 commits into from
Oct 6, 2022
Merged

fix: Update span ops #655

merged 2 commits into from
Oct 6, 2022

Conversation

cleptric
Copy link
Member

To unblock the work on Performance Issues, we have to update some span ops.
getsentry/sentry#37083

@cleptric cleptric self-assigned this Sep 23, 2022
@cleptric cleptric requested review from AbhiPrasad and a team September 23, 2022 08:57
k-fish
k-fish approved these changes Sep 23, 2022
View reviewed changes
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Looks good!

@ste93cry
Copy link
Collaborator

I chose the names of these operations because they are what the OpenTelemetry standard uses. Wouldn't it make more sense to support them in Sentry rather than shifting even more from a common standard which is well established in the industry?

@AbhiPrasad
Copy link
Member

OpenTelemetry doesn’t have the concept of a span op like we do, so fundamentally we’ll be diverging here anyway. Mapping otel span name -> description works for us, but span operation we should be free to work with.

We use the db prefix to generate performance issues: https://blog.sentry.io/2022/09/19/performance-issues-slow-you-can-act-on-quickly/

We are working on getting closer to the standard in general though. In getsentry/develop#686 I mapped out the transformation between opentelemetry <-> to sentry data, and we’re working on improving this.

@cleptric cleptric merged commit be492a2 into master Oct 6, 2022
@cleptric cleptric deleted the mh/span-ops branch October 6, 2022 21:35
@cleptric cleptric mentioned this pull request Oct 6, 2022
Merged
cleptric added a commit that referenced this pull request Oct 11, 2022
@cleptric
# This is a combination of 5 commits.
838cf45 
# This is the 1st commit message:

ref: Use constant for the SDK version

# This is the commit message #2:

docs: Update README.md & CONTRIBUTING.md (#650)

* docs: Update README.md & CONTRIBUTING.md

* Add slogan
# This is the commit message #3:

test: Add dsn to e2e test config (#657)

* test: Add dsn to e2e test config

* Add phpstan exclusion
# This is the commit message #4:

Fix Symfony 6 deprecations in E2E test (#656)


# This is the commit message #5:

fix: Update span ops (#655)
cleptric added a commit that referenced this pull request Oct 11, 2022
@cleptric
# This is a combination of 6 commits.
669afc9 
# This is the 1st commit message:

ref: Use constant for the SDK version

# This is the commit message #2:

docs: Update README.md & CONTRIBUTING.md (#650)

* docs: Update README.md & CONTRIBUTING.md

* Add slogan
# This is the commit message #3:

test: Add dsn to e2e test config (#657)

* test: Add dsn to e2e test config

* Add phpstan exclusion
# This is the commit message #4:

Fix Symfony 6 deprecations in E2E test (#656)

# This is the commit message #5:

fix: Update span ops (#655)

# This is the commit message #6:

docs: Update changelog (#658)
cleptric added a commit that referenced this pull request Oct 11, 2022
@cleptric
ref: Use constant for the SDK version
7f8e9db 
docs: Update README.md & CONTRIBUTING.md (#650)

* docs: Update README.md & CONTRIBUTING.md

* Add slogan

test: Add dsn to e2e test config (#657)

* test: Add dsn to e2e test config

* Add phpstan exclusion

Fix Symfony 6 deprecations in E2E test (#656)

fix: Update span ops (#655)

docs: Update changelog (#658)

Prepare release 4.3.1 (#659)

Update phpstan.neon

Add bump-version.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@k-fish k-fish k-fish approved these changes

@Jean85 Jean85 Awaiting requested review from Jean85

Assignees

@cleptric cleptric

Labels
Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cleptric @ste93cry @AbhiPrasad @k-fish