-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
JupyterLab Telemetry Design #8
Comments
Thanks, @jaipreet-s! This is great—I'm excited to see this move forward! Just a few comments...
I'm not sure if we're shooting for the Python and JS side to match exactly, but I wanted to bring up a minor difference I see here in the Handlers. The
(*thinking out loud here) Is the If it's part of the core server, what should the server do with these events? Assuming the source is trusted (a whole other converation, started in jupyter/telemetry#21), does the server pass incoming events hitting the If it's a server extension, jupyterlab-telemetry would have it's own EventLog that users/admins configure separately from the notebook server. That seems a bit cleaner... |
@Zsailer - Thanks for the feedback!
In the Python side, we got filtering with little at the handler level because of the in-built capabilities of the logging module. There isn't an equivalent I'm aware of in the TS world. I'm proposing we take this incrementally by starting with filtering at the entire
This will be the
Yes - the intention is to pass all whitelisted events to the server, and let the server EventLog configuration take care of which handlers to send the event to. This is useful in cases where an operator still wants to events occurring at the browser but keep common configuration across the server and browser. BTW I'm missing some permissions on this repo which I have at the |
@jaipreet-s I added you to the list of maintainers :) |
Right. Sounds good. I agree with taking an incremental approach. My question is whether we should start with a Handler object now (that lacks filtering etc. functionality for now) rather than using a function as the starting place. That way, we're not asking users to change from functions objects later? Or is this over-engineering 😆
Ah, yes. I forgot we added this in the notebook PR. I remember now... This means both EventLogs, server-side and client-side, must be configured by operators with the same set of whitelisted events. Then, validation happens twice, client-side then server-side. Correct? I'm not saying this is wrong, I'm just making sure I understand correctly :) |
The notion behind the function is because the "fan-out" of an event to each handler is managed by PhosphorJS signals, which accept a callback function for subscriber of an event. That is why the proposal was to start with a function so we get that aspect scalability for free. It looks very feasible to extend this to a
No its a good point - I've been thinking about this for other use-cases, and it may make sense to support regexes in the |
👍 great, that makes perfect sense to me. Thanks for explaining.
That's a cool idea. We could definitely implement something like this. I'd be curious to hear what @yuvipanda thinks. |
This proposal looks great to me! |
xref #9 which implements this |
Finally having a look at this. One question that comes up is about the relationship between handlers and allowed schemas. My understanding is that we would like an event log to be able to send different events to different handlers. For example, an operator might be sending events from one set of schemas to one handler, but an extension author may be collecting a different schema and sending those somewhere else. From the above discussion, it sounds like this usage case would be handled in the following manner:
A pro of this model is that there is a single authoritative source for the all I will have a look at the implementation now as well. Thanks for this work and discussion! |
👍
@ellisonbg |
This can be codified in the |
Good point!
…On Fri, Oct 4, 2019 at 3:06 PM Jaipreet Singh ***@***.***> wrote:
We should be able to query each handler for the schema subset they allow.
We can surface this information to users so they know where events are
being sent.
This can be codified in the Handler interface as well, for e.g. the
allowedSchemas can be an abstract property. The EventLog then can query
each handler's allowedSchemas property and take care of surfacing that to
the user
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8?email_source=notifications&email_token=AAAGXUEWRPVZUKL34UMBVP3QM646FA5CNFSM4IW4MMSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEANADIA#issuecomment-538575264>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAGXUEUOC22PFZZG2U3YBLQM646FANCNFSM4IW4MMSA>
.
--
Brian E. Granger
Principal Technical Program Manager, AWS AI Platform ([email protected])
On Leave - Professor of Physics and Data Science, Cal Poly
@ellisonbg on GitHub
|
Thanks for working on this, @jaipreet-s! I'm curious what you think of the schema naming recommendations here: https://github.com/jupyter/telemetry/blob/master/proposal/design.md#schema-naming-recommendations. I'd love for us to follow that for events we emit from inside the project, for the reasons outlined there. |
Agreed @yuvipanda. #11 tracks the implementation work for updating schema names. |
Hi all,
I'm doing a quick recap of the proposed design as we're going to begin implementation on this basis.
Publishing and consuming events
Publishers call the
recordEvent()
API on theEventLog
which emits event to each configured handlers. Handlers can be configured on the event log during instantiation. The EventLog will internally do schema validation and filterevents with allowed schemas.
The fan-out of an event to each handler is done using PhosphorJS Signals and each handler is simply a function with the signature
There will be an in-built handler which sends events to the
/eventlog
endpoint on the notebook server.JupyterLab commands
The EventLog has some configuration options which causes it to auto-subscribe to JupyterLab command executions (such as new Notebook opened, or terminal closed) and emit the to each handler. They still need to be whitelisting using the
allowedSchemas
property. This will be based off of Ian Rose's prototypeUser Transparency
To make it transparent to users what evens are being recorded, the EventLog will publish the set of allowed event schemas to the JupyterLab Settings Registry. Users will be able to view what events are being recorded and can also modify the settings to opt-out of certain events.
We'll have to deal with aggregating
allowedSchemas
from all EventLog instances but that is an implementation detail.cc @Zsailer @yuvipanda @zuoyuanh
The text was updated successfully, but these errors were encountered: