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

Added basic prometheus implementation #503

Closed

Conversation

swalker326
Copy link
Contributor

Added base prometheus implementation to track metrics.

Test Plan

navigate to localhost:9091/metrics to see query events being tracks

Checklist

  • Tests updated
  • Docs updated

Screenshots

Screenshot 2023-10-27 at 3 51 52 PM

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks! This is a great start! I have a few comments. Also, would you mind adding a new docs markdown file for this?

server/prometheus.server.ts Outdated Show resolved Hide resolved
server/index.ts Outdated Show resolved Hide resolved
server/index.ts Outdated Show resolved Hide resolved
app/utils/db.server.ts Outdated Show resolved Hide resolved
@swalker326
Copy link
Contributor Author

Thanks! This is a great start! I have a few comments. Also, would you mind adding a new docs markdown file for this?

Awesome, I resolved all the comments but I'm having an issue with the playwright run. I'll have more time this week to look at it, but if anyone wanted to take a look in the mean time. I'm fairly certain the issue revolves around how the server-build is importing the TS version of the prometheus.server.ts, in the JS file built file its trying to import prometheus.server.ts.

I will for sure work on getting some kind of docs updated too.

@kentcdodds
Copy link
Member

Sorry about that! I think this should work :)

@swalker326 swalker326 marked this pull request as ready for review October 31, 2023 18:20
Copy link
Contributor Author

@swalker326 swalker326 left a comment

Choose a reason for hiding this comment

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

I think I addressed all changes

server/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is super! Just a couple last things to tidy up and we're good to go. Thank you!

server/index.ts Outdated Show resolved Hide resolved
docs/metrics.md Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Sorry, just realized one more thing

server/index.ts Outdated Show resolved Hide resolved
@kentcdodds
Copy link
Member

Hey @swalker326, I just had a conversation with Milin (the CEO of Sentry) on something unrelated to this. But then this effort came up and Milin told me that Sentry has a lot of really awesome features that we can use to accomplish the same thing but better. I hope you don't mind if I pump the breaks on this until I can meet up with someone else from Sentry to evaluate what their offering is.

I always hate doing this kind of thing because I know you and @matt-winfield put a lot of effort into contributing these improvements to the stack. I hope you understand as we work hard to come up with the very best solution for everyone.

@matt-winfield
Copy link
Contributor

Excited to hear about what Sentry can bring to this area! More options are always a good thing.

And I just like to share the things I was going to be investigating anyway, hearing about cool alternatives is one of the main benefits - doesn't really matter to me if my original idea ends up being included 🙂

@swalker326
Copy link
Contributor Author

I share the same sentiment as @matt-winfield. No worries here at all, was fun to dig into it.

@jjaybrown
Copy link

Would @matt-winfield @swalker326 mind sharing the grafana dashboard which uses these metrics please?

@kentcdodds
Copy link
Member

Now that sentry is well supported,I think we can choose this

@kentcdodds kentcdodds closed this Jan 7, 2024
@kentcdodds
Copy link
Member

Thanks so much for the work on it though!

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

Successfully merging this pull request may close these issues.

4 participants