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

Add some native function to manipulate PromQL expressions. #534

Closed
wants to merge 4 commits into from

Conversation

tomwilkie
Copy link
Contributor

Signed-off-by: Tom Wilkie [email protected]

go.mod Show resolved Hide resolved
gopkg.in/yaml.v3 v3.0.0-20191010095647-fc94e3f71652
k8s.io/apimachinery v0.19.2
golang.org/x/crypto v0.0.0-20201208171446-5f87f3452ae9
gopkg.in/yaml.v2 v2.4.0
Copy link
Member

Choose a reason for hiding this comment

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

gopkg.in/yaml.v2 was pinned to v2.2.8 in #386 on purpose, becuase upgrading brings whitespace changes. @captncraig is this still relevant, or can we perform the upgrade by now?


// promQLRemoveByLabels updates PromQl expressions to remove all aggregation by labels
// eg `sum by(foo) (bar)` -> `sum (bar)`.
func promQLRemoveByLabels() *jsonnet.NativeFunction {
Copy link
Member

Choose a reason for hiding this comment

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

We are generally hesitant to introduce native functions, because they widen the gap between upstream Jsonnet and its ecosystem and Tanka further. We have seen them as a last resort for features that were impossible to implement otherwise, e.g. Helm support.

I'd be interested to hear what problem this function would solve and why it needs a native function (promql mutation too hard to implement in Jsonnet?). We need to keep in mind that if used in a mixin, this mixin would no longer evaluate in a non-Tanka context.

If we decide to merge this, it would need a Jsonnet wrapper along the lines of helm-util, so that Jsonnet code isn't strongly coupled to our implementation detail of this native function:

// prom-util.libsonnet
{
  trimLabelAggrs(expr):: std.native('promQLRemoveByLabels')(expr)
}

// somewhere else
local prom = (import "prom-util.libsonnet");
prom.trimLabelAggrs('sum by(foo) bar')

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree that having deep coupling to a specific use-case is setting the road to divergence with technology agnostic manifest management. I think it is not the business of Tanka to be the host of unrelated manifestation snippets.

For example, the parseYAML native function is an acceptable short term implementation until it is accepted in upstream Jsonnet. I do not think the same rationale applies to promQL snippet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

promql mutation too hard to implement in Jsonnet?

Exactly - building a parser etc would be far to hard in jsonnet.

We need to keep in mind that if used in a mixin, this mixin would no longer evaluate in a non-Tanka context.

Understood - but also consider I'm made mixtool and grizzly import tanka's native functions for this reason (and because yaml).

I'm managed to work around the need for these functions for now, but long term I think we will need something like this. Perhaps a more generic method for calling out to user code ie running a external binary?

Copy link
Member

Choose a reason for hiding this comment

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

There have been similar ideas and discussions upstream:

google/go-jsonnet#223
google/go-jsonnet#243

Judging on which terms those issues were closed, I don't feel much hope for these ideas being implemented upstream.

Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie tomwilkie closed this Mar 11, 2021
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.

6 participants