8000 feat: expose app insights as Prometheus metrics by mtojek · Pull Request #10346 · coder/coder · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 20 commits into from
Nov 7, 2023
Merged

feat: expose app insights as Prometheus metrics #10346

merged 20 commits into from
Nov 7, 2023

Conversation

mtojek
Copy link
Member
@mtojek mtojek commented Oct 19, 2023

Related: #9983

This PR adds support for exposing app insights metrics via the Prometheus endpoint.

@github-actions github-actions bot added the stale This issue is like stale bread. label Oct 27, 2023
@github-actions github-actions bot closed this Oct 30, 2023
@mtojek mtojek removed the stale This issue is like stale bread. label Nov 6, 2023
@mtojek mtojek reopened this Nov 6, 2023
@mtojek
Copy link
Member Author
mtojek commented Nov 6, 2023

Not stale yet, I was on PTO.

@mtojek mtojek requested a review from mafredri November 7, 2023 13:01
@mtojek mtojek marked this pull request as ready for review November 7, 2023 13:01
-- 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
)
Copy link
Member

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.

Copy link
Member Author
@mtojek mtojek Nov 7, 2023

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:

  1. Pulling unused columns from the database - I guess we can pull more as these fields do not carry heavy data.
  2. AND CASE WHEN COALESCE(array_length(@template_ids::uuid[], 1), 0) > 0 THEN w.template_id = ANY(@template_ids::uuid[]) ELSE TRUE END in GetTemplateAppInsights will be always false, so I just dropped it.
  3. 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 👍

@mtojek mtojek merged commit 0a55081 into main Nov 7, 2023
@mtojek mtojek deleted the 9983-apps-usage branch November 7, 2023 16:15
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0