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
feat: Accumulate Events #65
feat: Accumulate Events #65
Changes from 23 commits
ee49b2a
d07079d
ccece9b
f1da631
06ef5a9
525234c
b8cde72
882075f
6a59727
dd6a6cb
1802c4e
9e62d8f
ded115c
719519c
c459dcb
d23fcb6
db97c66
a28bc36
5791d84
98a7ab2
90b37ba
eb5ae38
f638eb8
1238a5f
6bd68c6
7c07a7d
4fe767b
9eea9a5
2722ec2
8eb34e9
d34928b
772018d
2313806
8c4f560
c999b06
fbaa4f3
dfc72df
c2c4277
a0242f9
ab218ab
3549175
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.
do we need this if we're using ts-node/esm loader?
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.
for some reason, it wants both.
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.
this change for running the tests without build should probably be in a separate PR
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.
yes, but I started when we did not have any testing in place, apologies, if you want I can spend sometime splitting these, but I hope it's fine for this time?
Large diffs are not rendered by default.
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.
+1. but should be separate PR
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.
should be a separate PR.
we will need a way to separate node and browser tests because of the countly import. if you're proposing we just find all tests in
node/
then we need to update thisThere 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.
tsconfig can have both node and browser types, just wondering if it still be a problem, moving for now.
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.
nit: 3p imports appearing after relative imports is so weird. we should eventually update with #74
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.
nit: should be separate PR
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 can look into #74 later and fix 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.
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'd prefer a generic approach. Without generics this is going to cause some typing issues.
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.
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.
nit: we could instantiate at the declaration
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.
this will break when using nodemetricsprovider
we should at least do
however, we should probably provide a way for node implementations to flush:
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.
nit: digest is a significantly overloaded term in our space and doesn't really communicate what this function does under most of the common definitions. Could we use
accumulate
(not accurate for new events) orprocess
(vague), or evenupdateEventData
(somewhat vague)?A better description in the jsdoc that doesn't use the term "digest" could help a lot here.
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.
should we have a flush_all?
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.
added with tests
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 think we should remove duration from the submitted event though, because countly doesn't really display them the way we think they should.. maybe we should chat about this while taking a peek at existing webui data in our countly server
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.
kinda-nit-kinda-not: calling metricsService directly here will force us to keep two classes up to date instead of having MetricsProvider be the primary abstraction. I'm fine with this for now because countly will probably be our metrics service for a while but we should try to stick to using MetricsProviders.addEvent() method instead.
i.e. constructor for this should take MetricsProvider instance as argument instead of the metricsService instance.
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.
we can discuss 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.
remove, we have this on line 44 already
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.
bad merge, one is from you one is from me lol.
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.
we need to revert this. appKey is not a valid argument for metricsService.init
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's not typechecked as expected, I fixed it like:
I should throw if
appKey
is missing?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.
yea we should. we can handle in a separate PR though.
This file was deleted.
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.
+1