Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 metrics counter feature tracking for NADE #1597
implement metrics counter feature tracking for NADE #1597
Changes from 14 commits
7f2618a
bd93cc3
4470cd6
b6101ab
945bc3c
5ae5bc7
fe6926d
61dd177
9145b67
9dccb93
949edf0
e8a3956
1316163
95ae58c
d24aedb
674decd
c41044e
f2def23
ef26a09
a17a0d4
6cc8c5f
b444cf2
0608844
f57f5a7
28912ab
618dc43
2326701
4b26b6f
b268e4e
8d0c4a4
65f1d9f
dde4337
a796c11
712b46a
23ccfac
fbb155c
5304633
dd2f34a
8af47f9
b6a272b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should switch these, so app comes first, so we can separate these easily? @sfc-gh-bdufour , @sfc-gh-turbaszek wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's here makes sense to me, I don't think I understand the suggestion. Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking, automated graphs will pick up anything under
features.
Do we want area-specific commands to also be part of that main graph, or do we want to adjust the hierarchy so thatapp
features
for instance, are tracked in separate graphs? In that case, 'app.features.snowpark_process' would make more sense:features.
and display them.app.features.
and display them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sfc-gh-mchok discussed with Bruno, and I think we can keep features first, but let's add something like 'features.global.' for the global features to keep the structure consistent, and be able to extract global features easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put this whole new logic into a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also refactored the usage here with this function, let me know if this works for you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a risk/side effect from calling this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the called functions and i do not believe there should be any, unless
self._jinja_env.parse
has side effects?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could throw error, but I guess it will throw them later anyways