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

feat: integration with sentry #6226

Merged
merged 7 commits into from
Jun 27, 2022
Merged

feat: integration with sentry #6226

merged 7 commits into from
Jun 27, 2022

Conversation

PsiACE
Copy link
Member

@PsiACE PsiACE commented Jun 24, 2022 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

usage

  • self host sentry or use SaaS
  • create a new project and get DSN
  • fill environment and run databend

sentry-databend

Changelog

  • New Feature

Related Issues

Fixes #6164

@vercel
Copy link

vercel bot commented Jun 24, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Jun 25, 2022 at 0:44AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Jun 24, 2022
@Xuanwo
Copy link
Member

Xuanwo commented Jun 24, 2022

Is it possible to integrate with sentry by open tracing?

@Xuanwo
Copy link
Member

Xuanwo commented Jun 24, 2022

We need something like https://pkg.go.dev/github.com/open-telemetry/opentelemetry-collector-contrib/exporter/sentryexporter#section-readme

PsiACE
PsiACE commented Jun 24, 2022
View reviewed changes
metasrv/src/bin/metasrv.rs Outdated Show resolved Hide resolved
common/tracing/src/logging.rs Outdated Show resolved Hide resolved
query/bin/databend-query.rs Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Jun 24, 2022

It doesn't make sense for applications to integrate tracing vendors one by one...

Also, it will add more maintain works.

@PsiACE
Copy link
Member Author

PsiACE commented Jun 24, 2022

Is it possible to integrate with sentry by open tracing?

I think it's possible, but I don't see this in practice at the moment.

It doesn't make sense for applications to integrate tracing vendors one by one...

You're right. I will explore other, simpler integration options.

@PsiACE PsiACE requested review from everpcpc and ZeaLoVe June 24, 2022 16:27
@PsiACE PsiACE marked this pull request as ready for review June 25, 2022 05:12
@PsiACE
Copy link
Member Author

PsiACE commented Jun 25, 2022

It looks like the Sentry Performance has some features similar to the Jaeger.

I think we can add an environment variable to enable the sentry tracing feature so that we can make better use of sentry.

@PsiACE
Copy link
Member Author

PsiACE commented Jun 25, 2022

Screenshot 2022-06-25 at 15-28-53 Performance - Event Details - sentry - Sentry

@PsiACE PsiACE requested a review from BohuTANG June 25, 2022 08:01
@BohuTANG
Copy link
Member

Please add some doc on how to profile databend using sentry.

@PsiACE
Copy link
Member Author

PsiACE commented Jun 25, 2022

Please add some doc on how to profile databend using sentry.

I feel it is better suited to tracing, now that the documentation has been added.

view doc: https://databend-git-fork-psiace-sentry-databend.vercel.app/doc/faq/how-to-tracing#error-tracking-and-performance-monitoring-with-sentry

BohuTANG
BohuTANG reviewed Jun 26, 2022
View reviewed changes
docs/doc/100-faq/20-how-to-tracing.md
Setting `SENTRY_TRACES_SAMPLE_RATE` greater than `0.0` will allow sentry to perform trace sampling, which will help set up performance monitoring.

```bash
DATABEND_SENTRY_DSN="<your-sentry-dsn>" SENTRY_TRACES_SAMPLE_RATE=1.0 LOG_LEVEL=DEBUG ./databend-query
Copy link
Member

Choose a reason for hiding this comment

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

log level is TRACE is ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is also possible, but debug is enough

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's ok to be consistent with jaeger's log_level.

dantengsky
dantengsky approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@dantengsky dantengsky left a comment

Choose a reason for hiding this comment

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

LGTM

mergify bot added a commit that referenced this pull request Jun 27, 2022
@mergify
Merge of #6226
db05b33
@mergify mergify bot mentioned this pull request Jun 27, 2022
Closed
8 tasks
@mergify mergify bot merged commit 616d983 into databendlabs:main Jun 27, 2022
@PsiACE PsiACE deleted the sentry branch June 27, 2022 09:27
This was referenced Jul 7, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantengsky dantengsky dantengsky approved these changes

@everpcpc everpcpc everpcpc approved these changes

@ZeaLoVe ZeaLoVe Awaiting requested review from ZeaLoVe

@BohuTANG BohuTANG Awaiting requested review from BohuTANG

@flaneur2020 flaneur2020 Awaiting requested review from flaneur2020

Assignees
No one assigned
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: integration with sentry
6 participants
@PsiACE @Xuanwo @BohuTANG @everpcpc @dantengsky @databend-bot