Skip to content
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

Include stacktrace into log messages #807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Nov 6, 2024

This would align behavior in Chrome and the spec and support the following use case:

As a user I want to filter out console messages from third party libraries. For this, I could use the URLs in the stack trace.

Related discussions:

Low-priority and we can discuss later.


Preview | Diff

Copy link
Contributor

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

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

I guess we are fine with the extra traffic, as long as the user is opting-in for the log messages.

@@ -10854,7 +10854,7 @@ Define the following [=console steps=] with |method|, |args|, and

1. Let |source| be the result of [=get the source=] given [=current Realm Record=].

1. If |method| is "<code>assert</code>", "<code>error</code>",
1. If |method| is "<code>assert</code>", "<code>error</code>", "<code>log</code>",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we perhaps do it for all methods?

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Include stacktrace into log messages.

The full IRC log of that discussion <AutomatedTester> topic: Include stacktrace into log messages
<AutomatedTester> github: https://github.com//pull/807
<orkon> q+
<AutomatedTester> ack next
<AutomatedTester> orkon: this is a PR/issue about stacktrace not being part of the log messages
<AutomatedTester> ... this was originally excluded but they have come up with a use case to ignore log messages if it comes from a "third party script".
<whimboo> q+
<AutomatedTester> ... I am now wondering if we should include stacktraces to all console logs?
<AutomatedTester> ack next
<jgraham> q+
<sadym> q+
<AutomatedTester> whimboo: I think it would be fine to add it. I think it would add it to console logging? I wonder if we want to all people to opt into this extra info as it could add a lot of info
<AutomatedTester> ack next
<AutomatedTester> jgraham: I was going to say something similar. I don't know if it is expensive to compute a stacktrace as it could be expensive
<AutomatedTester> ... this could lead into the subscription discussion later
<AutomatedTester> ... I think it would be good to have a way of config if you don't want debug messages you can exclude them and do we want to opt into stacktraces. It merits a discussion
<AutomatedTester> ack next
<AutomatedTester> sadym: I didn't mean that we add configuration to the subscription. I don't want to send them by default, just if the user subscribes it
<jdescottes> q+
<AutomatedTester> ... and users can limit the info being passed back with preload script
<AutomatedTester> ack next
<jgraham> q+
<AutomatedTester> jdescottes: I think that stacktraces are there for most things, have a way to add it already and support adding it in this scenario
<AutomatedTester> ack next
<AutomatedTester> jgraham: Let's add it by default and if we need filtering we can add that at a later stage
<whimboo> q+
<AutomatedTester> ack next
<orkon> q+
<AutomatedTester> whimboo: could someone submit a PR that updates and adds stacktrace
<AutomatedTester> ... or I can try do it next week
<AutomatedTester> ack next
<AutomatedTester> orkon: this is in the console spec already
<AutomatedTester> whimboo: we don't have this in firefox yet so I think we may need the spec changes
<AutomatedTester> q?
<whimboo> see also https://github.com/whatwg/console/issues/203
<AutomatedTester> whimboo: I have put a link to the issue above
<AutomatedTester> RRSAgent, make minutes
<AutomatedTester> q?
<RRSAgent> I have made the request to generate https://www.w3.org/2024/11/13-webdriver-minutes.html AutomatedTester
<AutomatedTester> sadym: we already send stacktrace for logEntry

@juliandescottes
Copy link
Contributor

As mentioned by @whimboo during the meeting, in Firefox we indeed only have stacktraces available for a few console message types. For log and warn, the only information available from platform at the moment is the filename/linenumber/columnnumber.

This would align behavior in Chrome and the spec.

Related discussions:

- puppeteer/puppeteer#13264
- whatwg/console#203
@OrKoN
Copy link
Contributor Author

OrKoN commented Nov 14, 2024

So I think this spec change does not depend on the behavior of the main Console spec because we are defining WebDriver BiDi-specific steps here. It should be possible to only compute stacktraces if there is a WebDriver BiDi connection and report them as WebDriver BiDi events without necessarily affecting the behavior in other instances of console use.

@OrKoN OrKoN requested a review from jgraham November 14, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues to be discussed by the working group needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants