Skip to content

refactor: update prometheus metrics config#38

Closed
nerdeveloper wants to merge 3 commits intochartmuseum:mainfrom
nerdeveloper:main
Closed

refactor: update prometheus metrics config#38
nerdeveloper wants to merge 3 commits intochartmuseum:mainfrom
nerdeveloper:main

Conversation

@nerdeveloper
Copy link
Copy Markdown
Member

Update Prometheus metric config from DISABLE_METRICS: true to ENABLE_METRICS: false

Copy link
Copy Markdown
Collaborator

@cbuto cbuto left a comment

Choose a reason for hiding this comment

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

thanks! 🎉 A few small changes needed but looks good.

Also, we’ll need to bump the chart version. Should this be a major version bump since this results in breaking changes @jdolitsky? Not sure how this was handled in the past!

DISABLE_STATEFILES: false
# disable Prometheus metrics
DISABLE_METRICS: true
ENABLE_METRICS: false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should also update the README

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

# disable use of index-cache.yaml
DISABLE_STATEFILES: false
# disable Prometheus metrics
DISABLE_METRICS: true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 I wonder if we should leave this here for a release so the depreciation warning is printed? Then remove it in a subsequent release?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea.. I am wondering if we should merge this now or wait until v0.15.0 is released. I suppose people could use this chart with canary / main, so perhaps we keep the old env var here for the time being

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want to believe this should have a milestone attached, Lmk if you want me to refactor this back to what it was.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@nerdeveloper @jdolitsky I feel like we should wait for v0.15.0 to be released, otherwise it might be a bit weird for both the ENABLE_METRICS and the DISABLE_METRICS config options to exist together.

Copy link
Copy Markdown
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

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

Can we bump the chart version to 3.7.0 ?

@nerdeveloper
Copy link
Copy Markdown
Member Author

Can we bump the chart version to 3.7.0 ?

Sure I can do that

nerdeveloper and others added 2 commits February 28, 2022 04:06
Co-authored-by: Casey Buto <cbuto22@gmail.com>
@nerdeveloper
Copy link
Copy Markdown
Member Author

nerdeveloper commented Feb 28, 2022

Also, Please do I need to update this content: https://github.com/chartmuseum/www/blob/main/content/docs/prometheus-metrics/_index.md?

cc @cbuto @jdolitsky

@cbuto
Copy link
Copy Markdown
Collaborator

cbuto commented Feb 28, 2022

Also, Please do I need to update this content: https://github.com/chartmuseum/www/blob/main/content/docs/prometheus-metrics/_index.md?

cc @cbuto @jdolitsky

yep! any docs that reference the metrics config should be updated (but only merged after 0.15.0 is released)

@nerdeveloper
Copy link
Copy Markdown
Member Author

@jdolitsky @cbuto I have opened PR and made changes to the website. Kindly check it out: chartmuseum/www#14

@cbuto
Copy link
Copy Markdown
Collaborator

cbuto commented Jun 3, 2022

@nerdeveloper can you rebase this with main and update the chart version?

I think this change will actually have in include the image bump as well when its time since its a breaking change

@cbuto
Copy link
Copy Markdown
Collaborator

cbuto commented Sep 27, 2022

handled in another PR

@cbuto cbuto closed this Sep 27, 2022
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.

3 participants