Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

feat(metrics): Add WASM feature flag #2517

Merged
merged 1 commit into from
Feb 11, 2021
Merged

feat(metrics): Add WASM feature flag #2517

merged 1 commit into from
Feb 11, 2021

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Feb 11, 2021

Description:
This change adds a feature flag to encapsulate the additional logic
needed for the stats WASM extension needed for SMI metrics. This is
deliberately not wired up in the main package yet so it can't be enabled
until the stats are fully integrated.


This PR is part of several which together extend Envoy to generate custom
metrics needed to implement the SMI metrics spec (#984).
Here is a map of the PRs as they're currently planned:

  1. (ref(injector): pass pod object to getEnvoySidecarContainerSpec #2479) ref(injector): pass pod object to getEnvoySidecarContainerSpec
  2. (feat(metrics): add WASM metrics source #2488) feat(metrics): add WASM metrics source
  3. (docs(metrics): Add docs for WASM stats #2503) docs(metrics): Add docs for WASM stats
  4. (YOU ARE HERE) feat(metrics): Add WASM feature flag
  5. feat(metrics): Add stats.wasm to workload pods
  6. feat(injector): Add name, workload name, workload kind to PodMetadata
  7. feat(metrics): Add source labels to WASM metrics
  8. feat(metrics): Add dest labels to WASM metrics
  9. feat(metrics): Add "unknown" for dest labels on local replies
  10. feat(metrics): clean up WASM metrics in Prometheus config
  11. feat(metrics): add flag to enable WASM metrics
  12. tests(e2e): add WASM metrics test

The full set of changes can be found here: main...nojnhuh:wasm-metrics

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@nojnhuh nojnhuh requested a review from a team as a code owner February 11, 2021 16:22
@@ -11,6 +11,8 @@ type OptionalFeatures struct {

// RoutesV2
RoutesV2 bool

WASMStatsFilepath string
Copy link
Member

Choose a reason for hiding this comment

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

This should ideally be the feature name instead of an attribute of the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was planning to set this directly to a command line flag for osm-controller, but it turns out I wasn't actually propagating that all the way through and was using basically a hardcoded value. I'll make this just a bool and keep the hardcoded path. I feel like if users want to swap out the .wasm or build their own container image, they know what they're doing enough to make that work themselves.

This change adds a feature flag to encapsulate the additional logic
needed for the stats WASM extension needed for SMI metrics. This is
deliberately not wired up in the main package yet so it can't be enabled
until the stats are fully integrated.

Co-authored-by: Edu Serra <eduser25@gmail.com>
Signed-off-by: Jon Huhn <johuhn@microsoft.com>
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.

3 participants