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

Sets cost estimation and tracking options #850

Merged
merged 8 commits into from Oct 27, 2023

Conversation

TristonianJones
Copy link
Collaborator

@TristonianJones TristonianJones commented Oct 26, 2023

Introduce library-based options for cost estimation and cost tracking

Breaking change - This change introduced a minor shift to ensure that all instances
of the interpreter.CostTracker were instantiated using interpreter.NewCostTracker().
This change revealed a bug where presence test, has(), costs were not being counted
when calling ContextEval() or Eval() with state tracking enabled. This bug has been
fixed, but revealed an inconsistency in expectations between the two method calls.

Note, these options only match against type-checked expressions. This is the
same behavior as is used internally within estimators; however, it is probably
worth indicating the cost estimates are only accurate against type-checked
expressions at a future date.

@cici37
Copy link
Collaborator

cici37 commented Oct 26, 2023

The cost might not set properly for other ext library as well. It might be out of scope for this PR but do we want to track it somewhere in an issue? (I am happy to create one if you are ok with it :) )

@TristonianJones
Copy link
Collaborator Author

@cici37 Yes, please, if you wouldn't mind filing an issue to introduce costs for the other extension libraries that would be great. Most of these costs are currently tracked within the K8s cost estimation extensions and should be moved back into the CEL libraries.

Copy link
Collaborator

@cici37 cici37 left a comment

Choose a reason for hiding this comment

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

Could we merge this into 0.17 release and cut another 0.17.7 patch release for this fix? I would like to avoid bumping to 0.18 only because of the bug fix if possible. :)

interpreter/runtimecost.go Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator Author

@cici37 PTAL

@cici37
Copy link
Collaborator

cici37 commented Oct 27, 2023

/lgtm
/approve
Thanks for helping with this!

@cici37
Copy link
Collaborator

cici37 commented Oct 27, 2023

/hold

@TristonianJones TristonianJones merged commit 39c2810 into google:master Oct 27, 2023
2 checks passed
@TristonianJones TristonianJones deleted the sets-costs-fix branch October 27, 2023 23:48
TristonianJones added a commit to TristonianJones/cel-go that referenced this pull request Oct 27, 2023
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.

None yet

3 participants