-
Notifications
You must be signed in to change notification settings - Fork 931
feat: expose app insights as Prometheus metrics #10346
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
Conversation
Not stale yet, I was on PTO. |
-- Subtract one minute because the series only contains the start time. | ||
AND s.start_time < (@end_time::timestamptz) - '1 minute'::interval | ||
GROUP BY s.start_time, w.template_id, was.user_id, was.agent_id, was.slug_or_port, wa.display_name, wa.slug | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to keep this part as similar as GetTemplateAppInsights
as possible, to reduce the maintenance burden/overhead of figuring out if a change should affect both? AFAICT the only change is removal of access_method
, but if we drop that there's also other simplifications we could do. Similarly, I don't think we gain much by dropping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasons why I decided to "fork" the query are:
- Pulling unused columns from the database - I guess we can pull more as these fields do not carry heavy data.
AND CASE WHEN COALESCE(array_length(@template_ids::uuid[], 1), 0) > 0 THEN w.template_id = ANY(@template_ids::uuid[]) ELSE TRUE END
inGetTemplateAppInsights
will be always false, so I just dropped it.- No need to perform
array_agg
in the last SELECT operation.
I would rather keep the forked query, but I could add a comment to justify above differences? Unless you are aware of a smart trick which isn't a FUNCTION.
EDIT:
I guess we can improve this in a follow-up 👍
Related: #9983
This PR adds support for exposing app insights metrics via the Prometheus endpoint.