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

chore: default observability.enable_prometheus to true #9222

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

NicholasBlaskey
Copy link
Contributor

@NicholasBlaskey NicholasBlaskey commented Apr 23, 2024

Ticket

Description

Default observability.enable_prometheus to true for convenience. There was no reason it should have been defaulted to false since the performance is negligible.

Test Plan

Run a devcluster with no prometheus config and see that

det dev curl /debug/prom/metrics returns metrics

Run a devcluster with

        observability:
          enable_prometheus: false

det dev curl /debug/prom/metrics returns not found

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@cla-bot cla-bot bot added the cla-signed label Apr 23, 2024
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.69%. Comparing base (9f6bbc9) to head (390c5d9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9222      +/-   ##
==========================================
- Coverage   44.70%   44.69%   -0.02%     
==========================================
  Files        1270     1270              
  Lines      155132   155136       +4     
  Branches     2437     2436       -1     
==========================================
- Hits        69351    69332      -19     
- Misses      85545    85568      +23     
  Partials      236      236              
Flag Coverage Δ
backend 41.52% <100.00%> (-0.05%) ⬇️
harness 64.31% <ø> (ø)
web 35.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/config/config.go 70.54% <100.00%> (+0.32%) ⬆️
master/test/testutils/fixtures.go 76.54% <100.00%> (+0.29%) ⬆️

... and 5 files with indirect coverage changes

Copy link

netlify bot commented Apr 23, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 390c5d9
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6627c85d549dac00088e28a6

@determined-ci determined-ci requested a review from a team April 23, 2024 14:06
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Apr 23, 2024
@NicholasBlaskey NicholasBlaskey marked this pull request as ready for review April 23, 2024 14:16
@NicholasBlaskey NicholasBlaskey requested a review from a team as a code owner April 23, 2024 14:16
Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

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

I'm glad you updated the wording on what "enabled_prometheus" does. Prometheus endpoints are present makes a lot more sense!

@NicholasBlaskey NicholasBlaskey enabled auto-merge (squash) April 23, 2024 14:41
@NicholasBlaskey NicholasBlaskey merged commit ac68df8 into main Apr 23, 2024
83 of 97 checks passed
@NicholasBlaskey NicholasBlaskey deleted the enable_prom_to_true branch April 23, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants