-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Create @elastic/analytics
package
#128407
Create @elastic/analytics
package
#128407
Conversation
de2dbe8
to
867b983
Compare
…aro/kibana into ebt/create-elastic-analytics-package
…tic-analytics-package
7e3f267
to
dcd11c5
Compare
…tic-analytics-package
…tic-analytics-package
…tic-analytics-package
@elasticmachine merge upstream |
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.
Found some other minor NITS, but overall LGTM!
We may want a second review from another team member though, to make sure we didn't miss anything.
* @param eventType the event type to check for the shipper | ||
*/ | ||
public isShipperOptedIn(shipperName: ShipperName, eventType?: EventType): boolean { | ||
const isGlobalOptedIn = this.isOptedIn(); |
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: I would exit fast here, instead of keeping this value and executing the following logic when unnecessary
if(!this.isOptedIn()) {
return false;
}
Same for isEventTypeOptedIn
Can also do the same with isShipperGloballyOptedIn
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.
Makes sense! Updated!
/** | ||
* Who emitted the event? It can be "client" or the name of the shipper. | ||
*/ | ||
source: 'client' | ShipperName; |
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: given client
is a special value here, I wonder if we should be forbidding to register a shipper with the client
name.
* Code to provide additional information about the success or failure. Examples are 200/400/504/ValidationError/UnknownError | ||
*/ | ||
code: string; |
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 somehow come with a defined list of values for this code? I'm guessing not given it can be specified by the shipper when encountering an error, but still asking.
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 leave it open for now to allow shippers to provide context about their failures. But yeah... we could potentially limit this in the future.
// TODO: We need to be able to edit sendTo once we resolve the telemetry config. | ||
// For now, we are relying on whether it's a distributable or running from source. | ||
sendTo: core.env.packageInfo.dist ? 'production' : 'staging', |
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 definitely be done in a follow-up, but thinking out loud, I wonder how we will be able to access the telemetry config from core
. Should we expose an API for another actor to call it with the config? Should core access the telemetry config directly (it's hacky and a bad isolation of concern, but technically core can access any plugin's config...)
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.
Yeah! I'm still wrapping my head about the best way to spread the config given the architecture of Kibana.
I've created #129014 to discuss and find out our preferred approach.
Just for traceability:
I wish we could offer a simple helper. However, the way this client works requires a plugin to register the FTR-helper shipper. If folks want to use them in their suites, they can change their config to import the plugin and the FTR helper. That was my intention with the last line of the README in that directory. WDYT? |
Yea, I'm not sure we can really do better than. Was just mentioning it because it would have been great, but if we can't, what you did in this PR is good. |
Changing the label from |
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.
LGTM 1 question about an @deprecated
comment
export interface RegisterShipperOpts { | ||
/** | ||
* List of event types that will be received only by this shipper. | ||
* @deprecated |
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.
It's explained in the following line:
Set as internal and deprecated until we come up with the best design for this. Not in the scope of the initial MVP.
At the moment we don't have a @beta-please-do-not-use
tag.
…tic-analytics-package
Agreed on Slack: as we are reaching the end of our sprint, I'll go ahead and merge this PR. Any comments from @lukeelmers will be addressed in future PRs. |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR creates the new
@elastic/analytics
package and all the tasks involved in this process:@elastic/analytics
packageanalyticsClient
APIscore.analytics
service both:server
public
server
public
server
public
server
public
Resolves #121995.
Checklist
Delete any items that are not applicable to this PR.
For maintainers