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

Add console.timeStamp() to the specification #236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bmeurer
Copy link

@bmeurer bmeurer commented Jul 12, 2024

This adds a minimal (vague) definition for the timeStamp() method, based on its current behavior across Chromium, Firefox, and Safari, and the MDN documentation1.

Ref: #140

  • At least two implementers are interested (and none opposed):
    • Google
    • Mozilla
    • Apple
  • Implementations already exist:
    • Chromium
    • Gecko
    • WebKit
  • MDN documentation already exists.
  • The top of this comment includes a clear commit message to use.

Preview | Diff

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/API/console/timestamp_static

This adds a minimal (vague) definition for the `timeStamp()` method,
based on its current behavior across Chromium, Firefox, and Safari,
and the MDN documentation[^1].

[^1]: https://developer.mozilla.org/en-US/docs/Web/API/console/timestamp_static

Ref: whatwg#140
@bmeurer
Copy link
Author

bmeurer commented Jul 12, 2024

@domfarolino Let's get the ball rolling here. 💪

@and-oli Please loop in your contacts with other browser vendors here.

@domfarolino
Copy link
Member

Hmm, it looks like you removed this line about tests from the pull request template? Can you add that back and maybe write some tests for this API? You can follow the pattern in https://github.com/web-platform-tests/wpt/tree/master/console. Note the state of testing is a little sad at this point, since these are basically just manual tests :(

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@@ -274,6 +275,12 @@ for plans to make {{console/timeEnd()}} and {{console/timeLog()}} formally repor
console when a given |label| does not exist in the associated <a>timer table</a>.
</p>

<h4 id="timestamp" oldids="timestamp-label,dom-console-timestamp" method for="console">timeStamp(|label|)</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should go in its own section about performance APIs? I guess this is kind of time related, but the existing timing APIs all have to do with keeping track of literal time stamps that get printed for the developer, where as timeStamp() seems to not interact with that timer table at all, and be more focused on interacting with a performance console.

This sounds very similar to console.markTimeline() actually. Am I correct in thinking it does not deal with actual raw times or timestamps at all?

(Also this is separate, and maybe I am dumb, but can you show me where the marked point actually shows up in the Performance panel's UI, when this API is called? I can't seem to find it when I call the API)

Copy link

Choose a reason for hiding this comment

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

can you show me where the marked point actually shows up in the Performance panel's UI, when this API is called?

In Chrome, any call to console.timeStamp that happens during a recording is shown inside the "Timings" track.
image

Copy link

Choose a reason for hiding this comment

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

and yeah at least in Chrome, you are right about timeStamp being independent from the other timing APIs in console. There, it simply adds an entry to the data buffer of the performance panel (a trace event)

Copy link
Member

Choose a reason for hiding this comment

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

Got it, can you confirm whether or not other browsers behave the same (or at least are as distinct from the other timing APIs as in Chrome)? That will help inform the tests we'll need for this I think.

Copy link
Author

Choose a reason for hiding this comment

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

According to MDN, console.timeStamp() is solely intended for the purpose of adding data to performance traces, and from my tests with Firefox, that's exactly what happens there.

@and-oli can you confirm for Safari?

Copy link
Author

Choose a reason for hiding this comment

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

@domfarolino Let me know if you want me to put this into a separate section on Performance APIs.

Copy link
Author

Choose a reason for hiding this comment

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

Friendly ping.

Copy link

Choose a reason for hiding this comment

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

Sorry for the delay. Safari also uses console.timeStamp solely to add markers to the Timelines tab: see docs

@domfarolino domfarolino added impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs tests Moving the issue forward requires someone to write tests labels Jul 12, 2024
@and-oli
Copy link

and-oli commented Jul 12, 2024

@dcrousso @nchevobbe

@@ -66,6 +66,7 @@ namespace console { // but see namespace object requirements below
undefined time(optional DOMString label = "default");
undefined timeLog(optional DOMString label = "default", any... data);
undefined timeEnd(optional DOMString label = "default");
undefined timeStamp(optional DOMString label = "default");
Copy link
Contributor

Choose a reason for hiding this comment

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

im not sure exactly the behavior of Chrome/Firefox/etc., but at least in WebKit there is no = "default" value (i.e. calling console.timeStamp() will show "Timestamp" in the UI)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Chromium appears to not use "default" at all. At least when provided with no string, it shows "TimeStamp: <empty string technically here>" in the UI which I think matches Safari, but I fear I am too dumb to figure out where in the "Timelines" UI calls to console.timeStamp() actually show up.

Screenshot 2024-07-21 at 11 09 01 PM

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just fixed V8 last week to have the = "default" value for all console.time*() APIs, including console.timeStamp(). Maybe we can update JSC as well to have consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

i kinda prefer it showing "Timestamp" in the UI, but i dont feel too strongly about it

we could always have the API use = "default" and then in the UI check if === "default" and if so show "Timestamp"

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so we can stick to = "default" for consistency here?

@@ -274,6 +275,12 @@ for plans to make {{console/timeEnd()}} and {{console/timeLog()}} formally repor
console when a given |label| does not exist in the associated <a>timer table</a>.
</p>

<h4 id="timestamp" oldids="timestamp-label,dom-console-timestamp" method for="console">timeStamp(|label|)</h4>

1. If the developer is recording a performance trace, add a single marker to the performance trace with the |label|.
Copy link
Contributor

Choose a reason for hiding this comment

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

im slightly concerned that this is the first time we've mentioned anything like "performance trace" in this spec. this feels a bit limiting to me (e.g. what if instead of showing a marker we'd like to put it in a table? how does this work with non-GUI environments? etc.). is there a way we can make this a bit more generic about just "capture a timestamp (relative to XYZ) for use in an implementation-defined way"?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, at first I strongly agreed, but when I looked into what Node and Deno do, they specifically mention the method is a noop unless hooked up to an inspector environment with a timeline UI. Maybe this isn't too bad to codify directly in the spec, since it matches reality with every single implementation, including non-browser ones.

But if people feel strongly, we can avoid that, making the normative text more vague, and adding some non-normative text mirroring what common developer documentation mentions about this API — namely that in browser/UI environments, it generally targets a performance-ey panel of sorts.

Copy link
Author

Choose a reason for hiding this comment

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

We can also make this more vague, but I thought it'd be useful to capture reasonable expectations for the API in the spec, so developers know that for conforming implementations

  1. this is a no-op when no trace is being recorded, and
  2. when a trace is being recorded, calls to console.timeStamp() will leave a mark in the trace in some form.

@bmeurer
Copy link
Author

bmeurer commented Jul 24, 2024

Hmm, it looks like you removed this line about tests from the pull request template? Can you add that back and maybe write some tests for this API? You can follow the pattern in https://github.com/web-platform-tests/wpt/tree/master/console. Note the state of testing is a little sad at this point, since these are basically just manual tests :(

Beyond testing that the function exists on the console object, how would we go about adding other (meaningful) tests?

@bmeurer
Copy link
Author

bmeurer commented Jul 30, 2024

Hmm, it looks like you removed this line about tests from the pull request template? Can you add that back and maybe write some tests for this API? You can follow the pattern in https://github.com/web-platform-tests/wpt/tree/master/console. Note the state of testing is a little sad at this point, since these are basically just manual tests :(

Beyond testing that the function exists on the console object, how would we go about adding other (meaningful) tests?

@domfarolino any thoughts here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs tests Moving the issue forward requires someone to write tests
Development

Successfully merging this pull request may close these issues.

4 participants