Add getter for thread type

Add getter for thread type

For the new cpu time metric, we would like to group threads with names like
"WorkerPool1/23" and "WorkerPool1/34" as the same "type". This CL adds a getter
for the type of a thread. The type is purely a function of the thread name, and
there is no way to set it manually.

R=benjhayden@chromium.org,tdresser@chromium.org

BUG=catapult:#3325

Review-Url: https://codereview.chromium.org/2800933002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5c399d8801ada5b88f1153fd3fb646bd7eb395e2

[tdresser, 2017-04-06 14:06:25]

LGTM with nits.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:315: const re = /[^0-9 |\/]+/;
Would this be clearer as /[A-Za-z]+/?

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:320: // thread name starts with a numeric
character or '/'. This is unlikely.
Does this ever happen? If not, should we throw here?

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread_test.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:187: var thread1 =
process.getOrCreateThread(1);
var -> let

[dproy, 2017-04-06 14:34:38]

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:315: const re = /[^0-9 |\/]+/;
On 2017/04/06 14:06:25, tdresser wrote:
> Would this be clearer as /[A-Za-z]+/?

Talked offline. Decided we will preserve unicode support in thread names for
now. 🎉

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:320: // thread name starts with a numeric
character or '/'. This is unlikely.
On 2017/04/06 14:06:25, tdresser wrote:
> Does this ever happen? If not, should we throw here?

I've never seen this happen sure let's throw an error here.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread_test.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:187: var thread1 =
process.getOrCreateThread(1);
On 2017/04/06 14:06:25, tdresser wrote:
> var -> let

Done.

[benjhayden, 2017-04-06 23:03:02]

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:305: * Returns a string that denotes the
'type' of a thread used for naming
The comment about what this is used for doesn't seem like it belongs at this
level of abstraction.
Maybe just this?
@return {string} substring of this.name from beginning to the first numeric or
symbolic character.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread_test.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:195: for (const thread of [thread1,
thread2, thread3]) {
Please try to avoid loops in tests.
https://www.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-best...
It's more verbose to spell out all the assertions, but it's easier to debug.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:196: assert.equal(thread.type,
'ThreadName');
Please use assert.strictEqual instead.
https://github.com/catapult-project/catapult/issues/3235

[dproy, 2017-04-06 23:26:23]

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread.html:305: * Returns a string that denotes the
'type' of a thread used for naming
On 2017/04/06 23:03:01, benjhayden wrote:
> The comment about what this is used for doesn't seem like it belongs at this
> level of abstraction.
> Maybe just this?
> @return {string} substring of this.name from beginning to the first numeric or
> symbolic character.

Done.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
File tracing/tracing/model/thread_test.html (right):

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:195: for (const thread of [thread1,
thread2, thread3]) {
On 2017/04/06 23:03:02, benjhayden wrote:
> Please try to avoid loops in tests.
>
https://www.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-best...
> It's more verbose to spell out all the assertions, but it's easier to debug.

Done.

https://codereview.chromium.org/2800933002/diff/1/tracing/tracing/model/threa...
tracing/tracing/model/thread_test.html:196: assert.equal(thread.type,
'ThreadName');
On 2017/04/06 23:03:02, benjhayden wrote:
> Please use assert.strictEqual instead.
> https://github.com/catapult-project/catapult/issues/3235

Done.

[dproy, 2017-04-07 18:43:15]

This is ready for review again. PTAL :)

[benjhayden, 2017-04-07 19:57:39]

lgtm

[dproy, 2017-04-07 20:37:48]

The CQ bit was checked by dproy@chromium.org

[dproy, 2017-04-07 20:37:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2800933002/#ps40001
(title: "Address comments by benjhayden@")

[commit-bot: I haz the power, 2017-04-07 20:37:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 21:01:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491597468500830,
"parent_rev": "fc76e21e74786edfd16ae1a9f4f661a22cb7b5be", "commit_rev":
"5c399d8801ada5b88f1153fd3fb646bd7eb395e2"}

[commit-bot: I haz the power, 2017-04-07 21:01:51]

Description was changed from

==========
Add getter for thread type

For the new cpu time metric, we would like to group threads with names like
"WorkerPool1/23" and "WorkerPool1/34" as the same "type". This CL adds a getter
for the type of a thread. The type is purely a function of the thread name, and
there is no way to set it manually.

R=benjhayden@chromium.org,tdresser@chromium.org

BUG=catapult:#3325
==========

to

==========
Add getter for thread type

For the new cpu time metric, we would like to group threads with names like
"WorkerPool1/23" and "WorkerPool1/34" as the same "type". This CL adds a getter
for the type of a thread. The type is purely a function of the thread name, and
there is no way to set it manually.

R=benjhayden@chromium.org,tdresser@chromium.org

BUG=catapult:#3325

Review-Url: https://codereview.chromium.org/2800933002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-04-07 21:01:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...