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

Chronicle v0.2.0 #443

Merged
merged 6 commits into from
Dec 6, 2023
Merged

Chronicle v0.2.0 #443

merged 6 commits into from
Dec 6, 2023

Conversation

AlexMapley
Copy link
Contributor

@AlexMapley AlexMapley commented Dec 5, 2023

Background

We jsut recently released the v0.1.0 chronicle chart with #441, and this is a followup adding some additional features and improvements.

Summary

  • Adds a configurable terminationGracePeriodSeconds value to our chronicle pods, though we use the standard 30 second period by default so this is functionally a noop
  • Fixes a bug in the chronicle config map preventing running pprof
  • Updates our chronicel ci values, testing some more edge cases
  • Updates our chronicle chart docs a bit as well
  • Bumps up the chronicle chart version to 0.2.0

@AlexMapley AlexMapley changed the title Chronicle 0.2.0 Chronicle v0.2.0 Dec 5, 2023
@AlexMapley AlexMapley marked this pull request as ready for review December 5, 2023 19:31
@@ -27,7 +27,7 @@ data:

[Profiling]
Enabled = {{ .Values.config.Profiling.Enabled }}
Listen = 3030
Listen = :3030
Copy link
Contributor Author

@AlexMapley AlexMapley Dec 5, 2023

Choose a reason for hiding this comment

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

This would cause a http server error: listen tcp: address 3030: missing port in address error log at bootup if you were using the default chart values from 0.1.0

{"level":"info","msg":"starting debug profiling server, listening at 3030","time":"2023-12-05T19:33:53.781Z"}
{"level":"error","msg":"http server error: listen tcp: address 3030: missing port in address","time":"2023-12-05T19:33:53.781Z"}
{"level":"info","msg":"storing metrics in parquet files in S3 at posit-chronicle-dev/v1/metrics","time":"2023-12-05T19:33:53.782Z"}
{"level":"info","msg":"storing metrics in parquet files at chronicle-data/v1/metrics","time":"2023-12-05T19:33:53.782Z"}
...

This was easy to miss as the error won't crash the server, and we were overwriting this value with :3030 in the ci values we used for templating and QA.

@@ -12,9 +12,8 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a template for the readme and the readme itself both checked in? Is this maybe a Helm thing? (In my experience, when you are using template files, you generally avoid checking in the rendered files. But I might be missing something here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want the README.md files here for readability purposes, otherwise users would have to generate the docs themselves with the just docs command.
We mention to users in https://helm.rstudio.com/ that they should look at the README.md files in this repo, from the github ui they render well too if you look at https://github.com/rstudio/helm/tree/main/charts/posit-chronicle for example:
Screenshot 2023-12-05 at 2 47 16 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - ok. I didn't quite connect that this was a public. (Too many years working in private/proprietary repos...) Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The README.md.gotmpl template files are able to pull in some shared helm doc markdown components we write in https://github.com/rstudio/helm/blob/main/charts/_templates.gotmpl, which are then shown across all of our chart readmes.

Copy link
Contributor

@markrtucker markrtucker left a comment

Choose a reason for hiding this comment

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

Changes look good. I just left one question about the use of the readme template, but that should not block merging.

@AlexMapley AlexMapley merged commit 489c66e into main Dec 6, 2023
1 check passed
@AlexMapley AlexMapley deleted the chronicle_0.2.0 branch December 6, 2023 17:14
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