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

implement a dsl for formulas #15

Closed
6 tasks done
martinm-nm opened this issue Aug 16, 2021 · 0 comments · Fixed by #19
Closed
6 tasks done

implement a dsl for formulas #15

martinm-nm opened this issue Aug 16, 2021 · 0 comments · Fixed by #19
Assignees
Labels
enhancement New feature or request

Comments

@martinm-nm
Copy link
Contributor

martinm-nm commented Aug 16, 2021

At the moment we treat formulas as text strings even though we have a dsl for queries:

query_all_reqs = Query(
    metric=Metric(name="aws.elb.request_count"),
    agg=Aggregation(func=AggFunc.SUM),
    name="reqs_all",
)

...

Request(
    formulas=[Formula(text="100 * (reqs_5xx / reqs_all)")],
    queries=[query_all_reqs, query],
    ...

This is unfortunate, because it means:

  • We need to parse the formula to check for syntax errors,
  • ...and unbound identifiers (in the example reqs_all attached to the Query has to match reqs_all in the formula text),
  • ...and invalid functions, eg. 100 * not_a_func(reqs_all)
  • Formulas are not structured the way queries are, they are not "programmable".

In terms of the language of formulas we have already captured it fully (to the best of my knowledge) via the grammar and the type definitions in libddog.metrics, so there aren't any unknowns here. We will now turn these types into a dsl so that formulas can be written like so:

query_all_reqs = Query(
    metric=Metric(name="aws.elb.request_count"),
    agg=Aggregation(func=AggFunc.SUM),
    name="reqs_all",  # this name no longer matters
)
reqs_all = query_all_reqs.identifier(name='reqs_all')

Formula(text=100 * (reqs_5xx / reqs_all))

Here, reqs_5xx and reqs_all are plain variables in Python instead. I'm unsure if we can support arithmetic expressions in natural syntax as shown, or whether we will need a custom representation for them like so:

Formula(text=Mul(100, (Div(reqs_5xx, reqs_all)))

Another approach would be to wrap Python literals instead and have these classes support the binary operators we need to support:

Formula(text=Int(100) * (reqs_5xx / reqs_all))

As part of this issue we should:

  • Implement the dsl for formulas
  • Add exhaustive unit tests for formulas
  • Make sure validation on formulas is user friendly (not just a naked assert)
  • Officially decouple the query dsl from the formula dsl, such that they have no common subclass. From now on we will treat these as separate languages.
  • Clean up all the types related to formulas in libddog.metrics and remove the ones that aren't actually useful.
  • Update the integration tests using formulas to use the dsl instead (textual formulas won't be supported anymore, anyway).
@martinm-nm martinm-nm added the enhancement New feature or request label Aug 16, 2021
@martinm-nm martinm-nm self-assigned this Aug 16, 2021
@martinm-nm martinm-nm mentioned this issue Aug 16, 2021
@martinm-nm martinm-nm linked a pull request Aug 16, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant