Skip to content

Add cell and notebook properties to the JupyterError event - #20

Open
cmarmo wants to merge 3 commits into
notebook-link:mainfrom
cmarmo:more-jupyter-error
Open

Add cell and notebook properties to the JupyterError event#20
cmarmo wants to merge 3 commits into
notebook-link:mainfrom
cmarmo:more-jupyter-error

Conversation

@cmarmo

@cmarmo cmarmo commented Aug 26, 2026

Copy link
Copy Markdown

Dear maintainers,
this pull request adds the id and metadata cell properties to the JupyterError event schema and to the reported metrics.

Thank you for considering it.

Comment thread src/emissions/jupyter-error.ts Outdated
Comment on lines +81 to +82
id: cell.model.toJSON().id as JSONValue,
metadata: cell.model.toJSON().metadata as JSONValue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hello,
Just asking a question at random, as I don't know the details of how cells are stored in memory.

Does cell.model.toJSON() triggers a full copy of all the data in the model?

If yes, then performance could be improved by:

  • calling cell.model.toJSON() only once
  • if possible: calling toJSON() selectively on the specific metadata attributes that will be exploited

Motivation: some of the cell metadata attributes may have large values, like for widget states. The model itself might (?) contain large data, like cell outputs).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, cell.model.toJSON() serializes completely. for id this is superfluous because id is always a primitive. For metadata it makes sense because it's better to make a copy than to pass by reference. But @cmarmo, I would ask, do you actually want everything inside metadata? Depending on the notebook this might have fields you don't care about. Either way, @nthiery's point is valid.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @afshin for your answer: in general I don't want to keep all the metadata, but the extension we are using now is configurable, so we can choose a list of metadata to be kept.
I wonder whether it is relevant to implement all the selection and configuration machinery here or create a new extension based on this one with custom events.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, we already know all metadata is safe json and we can't seek out specifics within that metadata at this layer of the stack, so I think your plan to emit the whole of the cell's metadata is okay here.

Comment thread src/emissions/jupyter-error.ts Outdated
/**
* The executed cell.
*/
cell: JSONObject;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would define the things you want JupyterError to provide under the cell key instead of making just a plain JSON object. It'll make it clearer what the emission is shipping.

@afshin

afshin commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

You're on the right track here! The build only failed because it needs a jlpm lint to catch linting issues.

@cmarmo

cmarmo commented Aug 27, 2026

Copy link
Copy Markdown
Author

I believe I have addressed the comments.
Also I have tentatively added "notebook" among the recorded information, hoping to retrieve the name/path of the executed notebook. I only have the id for now as I am unable to retrieve the path property without adding a NotebookTracker: is there another way?

@cmarmo cmarmo changed the title Add cell properties to the JupyterError event Add cell and notebook properties to the JupyterError event Aug 28, 2026
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.

3 participants