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

model: don't include metrics defaults in vmware-dev #1365

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Mar 4, 2021

Description of changes:

The vmware-dev model doesn't have settings.metrics because metricdog currently
relies on settings.aws.region.  If we update metricdog to support other
platforms and decide we want metrics there, we can link in this new
metrics.toml file.  Even then, it's useful to have separated in case other
people build variants where they don't want to send metrics.

Testing done:

In the vmware-dev variant, before the change, you would receive repeated messages on the console about not being able to deserialize settings because "settings.metrics" is unknown.

After, vmware-dev launches and runs fine, with no warnings, settings look good.

I also tested aws-k8s-1.17 and aws-ecs-1. Both launched and ran pods/tasks successfully.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

The vmware-dev model doesn't have settings.metrics because metricdog currently
relies on settings.aws.region.  If we update metricdog to support other
platforms and decide we want metrics there, we can link in this new
metrics.toml file.  Even then, it's useful to have separated in case other
people build variants where they don't want to send metrics.
@tjkirch tjkirch requested review from zmrow and webern March 4, 2021 23:19
@bcressey
Copy link
Contributor

bcressey commented Mar 5, 2021

In my work-in-progress branch for a vmware-k8s variant, I worked around this by defaulting to a "global" region:
bcressey@1f1aa9a#diff-175b2a1b6f79f2b0f7fa12696b8e02f14ae881ca7e556a43c7f2bbaf2abfb8f7

It felt like an error to me that vmware-dev didn't include MetricsSettings in its model. Maybe we don't care about deployment health for *-dev builds, though?

Here, I like the refactoring to make metrics optional, but I want us to figure out what the off-AWS, on-prem story should be. Perhaps we can defer that until the vmware-k8s-* builds are ready, but if so then I'd argue that aws-dev deserves the same treatment.

@tjkirch
Copy link
Contributor Author

tjkirch commented Mar 5, 2021

It felt like an error to me that vmware-dev didn't include MetricsSettings in its model.

I think in the long run it's an error, but at the present time we can't include it because metricdog doesn't support other platforms yet. I think we're planning on fixing that soon.

Maybe we don't care about deployment health for *-dev builds, though?

I think we should - it's a useful signal for the health of our components, which differ slightly in presence and composition across variants. I think we should work toward fixing metricdog rather than removing it in aws-dev.

I want us to figure out what the off-AWS, on-prem story should be.

I agree. I consider this a build fix, though, since vmware-dev can't run at all right now. @bcressey Are you OK with approving this to fix the build, and discussing future paths for metrics separately?

@tjkirch tjkirch requested a review from bcressey March 8, 2021 21:25
@zmrow
Copy link
Contributor

zmrow commented Mar 8, 2021

As per offline conversation, I'd vote for a "global" region until we know of something better.

@tjkirch tjkirch merged commit 1e03009 into bottlerocket-os:develop Mar 9, 2021
@tjkirch tjkirch deleted the vmware-no-metrics branch March 9, 2021 00:00
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