|
|
Created:
3 years, 7 months ago by chiniforooshan Modified:
3 years, 7 months ago CC:
chrome-grc-reviews_chromium.org, chromium-reviews, tracing-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing: The client lib of the tracing service
Any process that needs to connect to the tracing service and provide
trace events can just call TraceEventAgent::GetOrCreateInstance and
pass a connection to an implementation of the AgentSet interface.
A somewhat working prototype that includes the service
implementation:
https://codereview.chromium.org/2833873003/
Design doc (use @chromium.org account):
https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM
BUG=640235
Review-Url: https://codereview.chromium.org/2878533003
Cr-Commit-Position: refs/heads/master@{#472989}
Committed: https://chromium.googlesource.com/chromium/src/+/b3cad27b421b2aa9f6b0a89c80238480ba14023e
Patch Set 1 #Patch Set 2 : metadata generator #Patch Set 3 : . #
Total comments: 21
Patch Set 4 : review #
Total comments: 14
Patch Set 5 : review #
Total comments: 8
Patch Set 6 : review #
Total comments: 6
Patch Set 7 : review #Patch Set 8 : review #Patch Set 9 : fix a missing dep #Patch Set 10 : use thread checker macros #Messages
Total messages: 32 (12 generated)
Description was changed from ========== The client lib of the tracing service A somewhat working prototype: https://codereview.chromium.org/2833873003/ Design doc: https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 ========== to ========== tracing: The client lib of the tracing service Any process that needs to connect to the tracing service and provide trace events can just call TraceEventAgent::GetOrCreateInstance and pass a connection to an implementation of the AgentSet interface. A somewhat working prototype that includes the service implementation: https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 ==========
chiniforooshan@chromium.org changed reviewers: + oysteine@chromium.org, primiano@chromium.org
ptal.
Thanks, after you warmed me up with the memory_instrumentation servicification no I feel I understand way more :) this makes lot of sense, I have only some minor questions and comments. I think my only major doubt is what is the benefit of keeping the registry (AgentSet) and the Coordinator separated https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( Not sure what the final intended use of this, but I wonder if this should be really just CreateInstance? I fully understand that this pattern avoids to have a getinstance + initialize, which removes lot of problems. my only question is that if you intend to have the "get" semantic, do the client have to keep passing the agent_set? Also, if you really want the "get" semantic this starts looking racy. But I feel that what you want to do here is just invoking this method once in the various main()s, in which case is just a naming issue https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:52: bool report_categories_only, Hmm honestly this report_categories logic seems a bit odd here. I think understand what you are doing, but essentially here you are splashing the complexity on the client, which has to do start(report=true) and then immediately stop. Shouldn't we instead have a dedicated GetCategories method which, under the hoods, does the start+stop? Maybe that's a little bit cleaner? Or are there genuine cases where we want both the trace data AND the categories together? https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.h (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. is this "agent" going also to subsume the other base::trace_event::TracingAgent? If not might be better to pick a different name to avoid confusion (dunno, TracingClient?) https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:16: namespace resource_coordinator { i wonder if we should just keep this simple and just use the "tracing" namespace, like you did for memory_instrumentation . Seems it keeps the rest of the code more concise. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:22: typedef base::RepeatingCallback<std::unique_ptr<base::DictionaryValue>()> small nit: using MetadataGenerator = base... seems to be the preferred and more readable c++ way also can you call it MetadataGeneratorFunction or MetadataGeneratorCallback? otherwise, wihtout context, this looks like a class and makes the code harder to read. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:25: static TraceEventAgent* GetOrCreateInstance(mojom::AgentSetPtr agent_set); can you add a comment on what the argument is for? I can't figure out the agent_set straight away (at least until having seen the rest of the cl) https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:23: interface AgentSet { Out of curiosity, what is the reason why this is not just a method of Coordinator? Why do we need the two interfaces to be separated? (Don't have strong objections but just trying to understand the tradeoff... I wonder if I should do the same for memory_instrumentation). If this is separate, I think that AgentRegistry might be a better name. Until now, before I reched this line, I was thinking that AgentSet was a wrapper around std::set<Agent>
Thank you for reviewing this! https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( On 2017/05/11 15:31:07, Primiano Tucci wrote: > Not sure what the final intended use of this, but I wonder if this should be > really just CreateInstance? > I fully understand that this pattern avoids to have a getinstance + initialize, > which removes lot of problems. > my only question is that if you intend to have the "get" semantic, do the client > have to keep passing the agent_set? > Also, if you really want the "get" semantic this starts looking racy. > But I feel that what you want to do here is just invoking this method once in > the various main()s, in which case is just a naming issue I need a CreateInstance(agent_registry) which will be called in the main of different children, like the browser, renderers, or other services. I need a GetInstance() for adding MetadataGeneratorFunctions, probably just in the browser and/or service manager processes. The thing is that GetInstance will always be run right after CreateInstance because we can add MetadataGeneratorFunctions right at the beginning. So merging them will save a couple of lines of code. The client lib is not thread safe. Mojo calls are always made from the same thread on which the client is bound (i.e. the thread that ran the ctor). I added a ThreadChecker to make sure that AddMetadataGeneratorFunctions is called on the same thread, too. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:52: bool report_categories_only, On 2017/05/11 15:31:07, Primiano Tucci wrote: > Hmm honestly this report_categories logic seems a bit odd here. > I think understand what you are doing, but essentially here you are splashing > the complexity on the client, which has to do start(report=true) and then > immediately stop. > Shouldn't we instead have a dedicated GetCategories method which, under the > hoods, does the start+stop? > Maybe that's a little bit cleaner? > > Or are there genuine cases where we want both the trace data AND the categories > together? Done. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.h (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/11 15:31:07, Primiano Tucci wrote: > is this "agent" going also to subsume the other base::trace_event::TracingAgent? > If not might be better to pick a different name to avoid confusion (dunno, > TracingClient?) It's going to be a special base::trace_event::TracingAgent that sends back chrome trace events. I renamed it to ChromeAgent as we discussed in the design doc. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:16: namespace resource_coordinator { On 2017/05/11 15:31:07, Primiano Tucci wrote: > i wonder if we should just keep this simple and just use the "tracing" > namespace, like you did for memory_instrumentation . Seems it keeps the rest of > the code more concise. Done. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:22: typedef base::RepeatingCallback<std::unique_ptr<base::DictionaryValue>()> On 2017/05/11 15:31:07, Primiano Tucci wrote: > small nit: using MetadataGenerator = base... > seems to be the preferred and more readable c++ way > > also can you call it MetadataGeneratorFunction or MetadataGeneratorCallback? > otherwise, wihtout context, this looks like a class and makes the code harder to > read. Done. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:25: static TraceEventAgent* GetOrCreateInstance(mojom::AgentSetPtr agent_set); On 2017/05/11 15:31:07, Primiano Tucci wrote: > can you add a comment on what the argument is for? I can't figure out the > agent_set straight away (at least until having seen the rest of the cl) Done. https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:23: interface AgentSet { On 2017/05/11 15:31:07, Primiano Tucci wrote: > Out of curiosity, what is the reason why this is not just a method of > Coordinator? Why do we need the two interfaces to be separated? (Don't have > strong objections but just trying to understand the tradeoff... I wonder if I > should do the same for memory_instrumentation). > If this is separate, I think that AgentRegistry might be a better name. Until > now, before I reched this line, I was thinking that AgentSet was a wrapper > around std::set<Agent> Renamed to AgentRegistry. This is for security and privacy. We don't want to let all children to be able to collect and see trace events from other processes (e.g. can contain sensitive navigation data that cannot be given to a possibly-compromised child process). But, we want to let all children to be able to register themselves as trace providers. So, we need to provide the registration to all children but coordination ability only to trusted processes (i.e. the browser). The design doc linked in the CL document has a section about this.
sorry for the delay. I meant to send this reply this morning, but then I started talking with people and I forgot to click on "publish". Only minor comments left. the rest lgtm with those addressed. Oystein, thoughts? https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( On 2017/05/15 20:44:11, chiniforooshan wrote: > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > Not sure what the final intended use of this, but I wonder if this should be > > really just CreateInstance? > > I fully understand that this pattern avoids to have a getinstance + > initialize, > > which removes lot of problems. > > my only question is that if you intend to have the "get" semantic, do the > client > > have to keep passing the agent_set? > > Also, if you really want the "get" semantic this starts looking racy. > > But I feel that what you want to do here is just invoking this method once in > > the various main()s, in which case is just a naming issue > > I need a CreateInstance(agent_registry) which will be called in the main of > different children, like the browser, renderers, or other services. I need a > GetInstance() for adding MetadataGeneratorFunctions, probably just in the > browser and/or service manager processes. The thing is that GetInstance will > always be run right after CreateInstance because we can add > MetadataGeneratorFunctions right at the beginning. So merging them will save a > couple of lines of code. If this is the case can you just make this TraceEventAgent* CreateInstance(agent_set) and inside that check that this is called only once. If we then need an extra Get, we can create a separate getter-only TraceEventAgent* GetInstance(). The reason I am advocating about this is because in the past we have been quite bitten by the GetInstance() model that lazily brings up the MemoryDumpManager / TracingController / TraceLog. Essentially today there are a bounch of places that invoke that and is impossible to easily understand in which order they are created. In fact depending on the combination of --enable-heap-profiling and --startup-trace that you use they get initialized in different order (because the GetInstance() get called in a different sequence) and that has been a huge mainteinance burdern. Hence my suggestion of making it clear when the instance is created (and returned, that is fine) vs just accessed.. > > The client lib is not thread safe. Mojo calls are always made from the same > thread on which the client is bound (i.e. the thread that ran the ctor). I added > a ThreadChecker to make sure that AddMetadataGeneratorFunctions is called on the > same thread, too. Ack, thanks https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.h (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/15 20:44:12, chiniforooshan wrote: > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > is this "agent" going also to subsume the other > base::trace_event::TracingAgent? > > If not might be better to pick a different name to avoid confusion (dunno, > > TracingClient?) > > It's going to be a special base::trace_event::TracingAgent that sends back > chrome trace events. I renamed it to ChromeAgent as we discussed in the design > doc. tracing::ChromeAgent SGTM (or if you want even just tracing::Agent) https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:23: interface AgentSet { On 2017/05/15 20:44:12, chiniforooshan wrote: > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > Out of curiosity, what is the reason why this is not just a method of > > Coordinator? Why do we need the two interfaces to be separated? (Don't have > > strong objections but just trying to understand the tradeoff... I wonder if I > > should do the same for memory_instrumentation). > > If this is separate, I think that AgentRegistry might be a better name. Until > > now, before I reched this line, I was thinking that AgentSet was a wrapper > > around std::set<Agent> > > Renamed to AgentRegistry. > > This is for security and privacy. We don't want to let all children to be able > to collect and see trace events from other processes (e.g. can contain sensitive > navigation data that cannot be given to a possibly-compromised child process). > But, we want to let all children to be able to register themselves as trace > providers. So, we need to provide the registration to all children but > coordination ability only to trusted processes (i.e. the browser). > > The design doc linked in the CL document has a section about this. Yup, sorry I added this comment before getting to that part of the doc. Makes perfect sense (maybe clarify in a readme or in the comment). Knowing myself I am sure that 3 months from now I am going to ping you and ask "hey why did we split the two and they are not the same?" :P https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent.cc (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.cc:92: void ChromeAgent::SendChunk( minor comment: I'd probably call this OnTraceLogFlush. The problem I have with SendChunk is that is not immediate to figure out the direction (who sends the chunk where) https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.cc:95: DCHECK(recorder_); I think the pattern is to not add dhcecks if we are going to dereference the pointer below and crash anyways. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent.h (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:9: #include "base/memory/ref_counted_memory.h" not sure you need this https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:11: #include "base/values.h" I think you can just forward declare base::DictionaryValue and move the include to the .cc file (if for any reason doesn't work because of the callback ignore this comment) https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:19: class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeAgent Ahhhhh now I understand. this is not a generic agent interface. This is the actual chrome traece event agent. sorry i didn't understand this in the doc. I agree with you that ChromeTraceEventAgent, or ChromeTracingAgent is also a stronger and more indicative name. As you prefer though, I feel I already asked you to rename this once. sorry my misunderstanding in the doc,I thought we were talking about "how should the generic interface be called?" https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc:57: EXPECT_TRUE(event_dict->GetString("cat", &category)); instead of reparsing all this and depending on the serialization format, you can use TraceEventAnalyzer (see examples in memory_dump_manager_unittest.cc) https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc:67: LOG(ERROR) << "meta call"; looks like a leftover from local debugging :)
https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > On 2017/05/15 20:44:11, chiniforooshan wrote: > > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > > Not sure what the final intended use of this, but I wonder if this should be > > > really just CreateInstance? > > > I fully understand that this pattern avoids to have a getinstance + > > initialize, > > > which removes lot of problems. > > > my only question is that if you intend to have the "get" semantic, do the > > client > > > have to keep passing the agent_set? > > > Also, if you really want the "get" semantic this starts looking racy. > > > But I feel that what you want to do here is just invoking this method once > in > > > the various main()s, in which case is just a naming issue > > > > I need a CreateInstance(agent_registry) which will be called in the main of > > different children, like the browser, renderers, or other services. I need a > > GetInstance() for adding MetadataGeneratorFunctions, probably just in the > > browser and/or service manager processes. The thing is that GetInstance will > > always be run right after CreateInstance because we can add > > MetadataGeneratorFunctions right at the beginning. So merging them will save a > > couple of lines of code. > > If this is the case can you just make this TraceEventAgent* > CreateInstance(agent_set) and inside that check that this is called only once. > If we then need an extra Get, we can create a separate getter-only > TraceEventAgent* GetInstance(). > The reason I am advocating about this is because in the past we have been quite > bitten by the GetInstance() model that lazily brings up the MemoryDumpManager / > TracingController / TraceLog. Essentially today there are a bounch of places > that invoke that and is impossible to easily understand in which order they are > created. In fact depending on the combination of --enable-heap-profiling and > --startup-trace that you use they get initialized in different order (because > the GetInstance() get called in a different sequence) and that has been a huge > mainteinance burdern. > > Hence my suggestion of making it clear when the instance is created (and > returned, that is fine) vs just accessed.. But, this is not guaranteed to be called once. This will be called in the start of services and if we bundle several services in one process, this will be called several times, until we have a better way of initializing these one-per-process libs in the service manager (or is there a way today for this?). I tried to explain this in the design doc, but I should make the explanation more clear). https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:23: interface AgentSet { On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > On 2017/05/15 20:44:12, chiniforooshan wrote: > > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > > Out of curiosity, what is the reason why this is not just a method of > > > Coordinator? Why do we need the two interfaces to be separated? (Don't have > > > strong objections but just trying to understand the tradeoff... I wonder if > I > > > should do the same for memory_instrumentation). > > > If this is separate, I think that AgentRegistry might be a better name. > Until > > > now, before I reched this line, I was thinking that AgentSet was a wrapper > > > around std::set<Agent> > > > > Renamed to AgentRegistry. > > > > This is for security and privacy. We don't want to let all children to be able > > to collect and see trace events from other processes (e.g. can contain > sensitive > > navigation data that cannot be given to a possibly-compromised child process). > > But, we want to let all children to be able to register themselves as trace > > providers. So, we need to provide the registration to all children but > > coordination ability only to trusted processes (i.e. the browser). > > > > The design doc linked in the CL document has a section about this. > > Yup, sorry I added this comment before getting to that part of the doc. Makes > perfect sense (maybe clarify in a readme or in the comment). Knowing myself I am > sure that 3 months from now I am going to ping you and ask "hey why did we split > the two and they are not the same?" :P Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent.cc (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.cc:92: void ChromeAgent::SendChunk( On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > minor comment: I'd probably call this OnTraceLogFlush. > The problem I have with SendChunk is that is not immediate to figure out the > direction (who sends the chunk where) Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.cc:95: DCHECK(recorder_); On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > I think the pattern is to not add dhcecks if we are going to dereference the > pointer below and crash anyways. Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent.h (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:9: #include "base/memory/ref_counted_memory.h" On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > not sure you need this For base::RefCountedString. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:11: #include "base/values.h" On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > I think you can just forward declare base::DictionaryValue and move the include > to the .cc file (if for any reason doesn't work because of the callback ignore > this comment) Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent.h:19: class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeAgent On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > Ahhhhh now I understand. this is not a generic agent interface. This is the > actual chrome traece event agent. > sorry i didn't understand this in the doc. > I agree with you that ChromeTraceEventAgent, or ChromeTracingAgent is also a > stronger and more indicative name. > As you prefer though, I feel I already asked you to rename this once. sorry my > misunderstanding in the doc,I thought we were talking about "how should the > generic interface be called?" Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc (right): https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc:57: EXPECT_TRUE(event_dict->GetString("cat", &category)); On 2017/05/16 04:45:12, Primiano (slow - traveling) wrote: > instead of reparsing all this and depending on the serialization format, you can > use TraceEventAnalyzer (see examples in memory_dump_manager_unittest.cc) Done. https://codereview.chromium.org/2878533003/diff/60001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_agent_unittest.cc:67: LOG(ERROR) << "meta call"; On 2017/05/16 04:45:12, Primiano (slow - traveling) wrote: > looks like a leftover from local debugging :) Done.
Pinging Oystein :)
chiniforooshan@chromium.org changed reviewers: + kenrb@chromium.org
+Ken for tracing.mojom
https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:50: metadata_generator_functions_.push_back(generator); Would a DCHECK(recorder_); make sense here? https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:70: base::trace_event::TraceLog::GetInstance()->Flush(base::Bind( This can get called from ~ChromeTraceEventAgent(), passing base::Unretained(this) to a callback seems bad. Even when it's not called from the destructor, couldn't this class be destructed by the messagepipe being closed at any time? https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:82: auto status = base::trace_event::TraceLog::GetInstance()->GetStatus(); nit: Since you're referring to fields of "status" below, rather than just using it opaquely, I would specify the type rather than using auto. https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:103: recorder_->AddMetadata(std::move(metadata)); Any chance we could add the metadata first? (Basically using a flag to only run through the generator functions once). There's situations where it's advantageous to be able to pull out the metadata from a large trace file without needing to read/decompress the entire file (plus we'd know more about truncated files in, say, Slow Reports).
https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( On 2017/05/16 15:14:44, chiniforooshan wrote: > On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > > On 2017/05/15 20:44:11, chiniforooshan wrote: > > > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > > > Not sure what the final intended use of this, but I wonder if this should > be > > > > really just CreateInstance? > > > > I fully understand that this pattern avoids to have a getinstance + > > > initialize, > > > > which removes lot of problems. > > > > my only question is that if you intend to have the "get" semantic, do the > > > client > > > > have to keep passing the agent_set? > > > > Also, if you really want the "get" semantic this starts looking racy. > > > > But I feel that what you want to do here is just invoking this method once > > in > > > > the various main()s, in which case is just a naming issue > > > > > > I need a CreateInstance(agent_registry) which will be called in the main of > > > different children, like the browser, renderers, or other services. I need a > > > GetInstance() for adding MetadataGeneratorFunctions, probably just in the > > > browser and/or service manager processes. The thing is that GetInstance will > > > always be run right after CreateInstance because we can add > > > MetadataGeneratorFunctions right at the beginning. So merging them will save > a > > > couple of lines of code. > > > > If this is the case can you just make this TraceEventAgent* > > CreateInstance(agent_set) and inside that check that this is called only once. > > If we then need an extra Get, we can create a separate getter-only > > TraceEventAgent* GetInstance(). > > The reason I am advocating about this is because in the past we have been > quite > > bitten by the GetInstance() model that lazily brings up the MemoryDumpManager > / > > TracingController / TraceLog. Essentially today there are a bounch of places > > that invoke that and is impossible to easily understand in which order they > are > > created. In fact depending on the combination of --enable-heap-profiling and > > --startup-trace that you use they get initialized in different order (because > > the GetInstance() get called in a different sequence) and that has been a huge > > mainteinance burdern. > > > > Hence my suggestion of making it clear when the instance is created (and > > returned, that is fine) vs just accessed.. > > But, this is not guaranteed to be called once. This will be called in the start > of services and if we bundle several services in one process, this will be > called several times, until we have a better way of initializing these > one-per-process libs in the service manager Okay I think I am still missing this point. Realistically, even before we have a way to initialize this in the service manager, I expect this to be called only from child_thread::main. Is there a realistic case where we risk to initialize this more than once? If not we should just have a CreateInstance(...) which has a DCHECK against double-initialization, and revisit that if we have an actual use case.
https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( On 2017/05/17 00:28:17, Primiano (slow - traveling) wrote: > On 2017/05/16 15:14:44, chiniforooshan wrote: > > On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > > > On 2017/05/15 20:44:11, chiniforooshan wrote: > > > > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > > > > Not sure what the final intended use of this, but I wonder if this > should > > be > > > > > really just CreateInstance? > > > > > I fully understand that this pattern avoids to have a getinstance + > > > > initialize, > > > > > which removes lot of problems. > > > > > my only question is that if you intend to have the "get" semantic, do > the > > > > client > > > > > have to keep passing the agent_set? > > > > > Also, if you really want the "get" semantic this starts looking racy. > > > > > But I feel that what you want to do here is just invoking this method > once > > > in > > > > > the various main()s, in which case is just a naming issue > > > > > > > > I need a CreateInstance(agent_registry) which will be called in the main > of > > > > different children, like the browser, renderers, or other services. I need > a > > > > GetInstance() for adding MetadataGeneratorFunctions, probably just in the > > > > browser and/or service manager processes. The thing is that GetInstance > will > > > > always be run right after CreateInstance because we can add > > > > MetadataGeneratorFunctions right at the beginning. So merging them will > save > > a > > > > couple of lines of code. > > > > > > If this is the case can you just make this TraceEventAgent* > > > CreateInstance(agent_set) and inside that check that this is called only > once. > > > If we then need an extra Get, we can create a separate getter-only > > > TraceEventAgent* GetInstance(). > > > The reason I am advocating about this is because in the past we have been > > quite > > > bitten by the GetInstance() model that lazily brings up the > MemoryDumpManager > > / > > > TracingController / TraceLog. Essentially today there are a bounch of places > > > that invoke that and is impossible to easily understand in which order they > > are > > > created. In fact depending on the combination of --enable-heap-profiling and > > > --startup-trace that you use they get initialized in different order > (because > > > the GetInstance() get called in a different sequence) and that has been a > huge > > > mainteinance burdern. > > > > > > Hence my suggestion of making it clear when the instance is created (and > > > returned, that is fine) vs just accessed.. > > > > But, this is not guaranteed to be called once. This will be called in the > start > > of services and if we bundle several services in one process, this will be > > called several times, until we have a better way of initializing these > > one-per-process libs in the service manager > > Okay I think I am still missing this point. Realistically, even before we have a > way to initialize this in the service manager, I expect this to be called only > from child_thread::main. > Is there a realistic case where we risk to initialize this more than once? If > not we should just have a CreateInstance(...) which has a DCHECK against > double-initialization, and revisit that if we have an actual use case. Agree, let's differ this discussion to the CL that needs this. For now, I just add a GetInstance() method. Creator of this lib, e.g. ChildThreadImpl, should manage its lifetime, which should be just storing it in an internal std::unique_ptr variable. There is a DCHECK that verifies no more than one instance is live at any time in each process. https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... File services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc (right): https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:50: metadata_generator_functions_.push_back(generator); On 2017/05/16 21:19:05, oystein wrote: > Would a DCHECK(recorder_); make sense here? Not really. Recorders are given at StartTracing time and are destroyed after flushing events is complete. Metadata generator functions are added at the initialisation time only. The generators are then called every time tracing is stopped after flushing all trace events (please see OnTraceLogFlush), at which time we must have a recorder (but not necessarily here). https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:70: base::trace_event::TraceLog::GetInstance()->Flush(base::Bind( On 2017/05/16 21:19:06, oystein wrote: > This can get called from ~ChromeTraceEventAgent(), passing > base::Unretained(this) to a callback seems bad. Even when it's not called from > the destructor, couldn't this class be destructed by the messagepipe being > closed at any time? Good point. Changed the destructor to not calling this anymore. The lifetime of this object is not related to the state of the internal variable binding_. mojo::Binding does not take ownership of the implementation (i.e. ChromeTraceEventAgent); mojo::StrongBinding does. In the use cases that I have in mind, this client library is created close to when a process (e.g. a renderer) is created and is destroyed when the process is destroyed (of course, tests are exceptions). So, if we ignore the shutdown sequence at the very end of a process life, the ChromeTraceEventAgent instance should live as long as the TraceLog instance lives. https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:82: auto status = base::trace_event::TraceLog::GetInstance()->GetStatus(); On 2017/05/16 21:19:05, oystein wrote: > nit: Since you're referring to fields of "status" below, rather than just using > it opaquely, I would specify the type rather than using auto. Done. https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:103: recorder_->AddMetadata(std::move(metadata)); On 2017/05/16 21:19:05, oystein wrote: > Any chance we could add the metadata first? (Basically using a flag to only run > through the generator functions once). There's situations where it's > advantageous to be able to pull out the metadata from a large trace file without > needing to read/decompress the entire file (plus we'd know more about truncated > files in, say, Slow Reports). The agent can write the metadata first; but the difficulty is that the tracing service should wait for all children to send metadata before it can write metadata to the sink (a trace file for example). But, it can start writing trace events as soon as at least one of the children starts sending trace events. So, if we want to write the metadata first, we have to wait for all children to reply and buffer all trace events that we have received so far so that we can write them to the output stream after writing the metadata which will not be very efficient memory wise and CPU wise. An alternative way is to somehow mark or hard-code children that we expect to receive metadata from them (the browser maybe?) and just wait for them. I changed this to write the metadata first. I will add a section to discuss this in the design doc too. We can make a decision on the CL that implements the coordinator to whether write the metadata to the sink first or start writing events as soon as possible.
mojom lgtm, with a nit and a question. https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:25: // reasones: although we want to let almost every process be able to send nit: s/reasones/reasons https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:60: interface Coordinator { How do you prevent renderer processes from being able to connect to this interface? I had a quick look through the prototype and couldn't see anything -- is the intent to add a ConnectionFilter?
https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:25: // reasones: although we want to let almost every process be able to send On 2017/05/17 21:24:02, kenrb wrote: > nit: s/reasones/reasons Done. https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:60: interface Coordinator { On 2017/05/17 21:24:02, kenrb wrote: > How do you prevent renderer processes from being able to connect to this > interface? I had a quick look through the prototype and couldn't see anything -- > is the intent to add a ConnectionFilter? Isn't it enough that we don't "provide" the Coordinator interface to the renderer in the tracing service manifest or in content/public/app/mojo/content_browser_manifest.json and only provide the AgentRegistry interface?
lgtm and thanks! On 2017/05/17 at 06:33:47, chiniforooshan wrote: > https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... > File services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc (right): > > https://codereview.chromium.org/2878533003/diff/40001/services/resource_coord... > services/resource_coordinator/public/cpp/tracing/trace_event_agent.cc:19: TraceEventAgent* TraceEventAgent::GetOrCreateInstance( > On 2017/05/17 00:28:17, Primiano (slow - traveling) wrote: > > On 2017/05/16 15:14:44, chiniforooshan wrote: > > > On 2017/05/16 04:45:11, Primiano (slow - traveling) wrote: > > > > On 2017/05/15 20:44:11, chiniforooshan wrote: > > > > > On 2017/05/11 15:31:07, Primiano Tucci wrote: > > > > > > Not sure what the final intended use of this, but I wonder if this > > should > > > be > > > > > > really just CreateInstance? > > > > > > I fully understand that this pattern avoids to have a getinstance + > > > > > initialize, > > > > > > which removes lot of problems. > > > > > > my only question is that if you intend to have the "get" semantic, do > > the > > > > > client > > > > > > have to keep passing the agent_set? > > > > > > Also, if you really want the "get" semantic this starts looking racy. > > > > > > But I feel that what you want to do here is just invoking this method > > once > > > > in > > > > > > the various main()s, in which case is just a naming issue > > > > > > > > > > I need a CreateInstance(agent_registry) which will be called in the main > > of > > > > > different children, like the browser, renderers, or other services. I need > > a > > > > > GetInstance() for adding MetadataGeneratorFunctions, probably just in the > > > > > browser and/or service manager processes. The thing is that GetInstance > > will > > > > > always be run right after CreateInstance because we can add > > > > > MetadataGeneratorFunctions right at the beginning. So merging them will > > save > > > a > > > > > couple of lines of code. > > > > > > > > If this is the case can you just make this TraceEventAgent* > > > > CreateInstance(agent_set) and inside that check that this is called only > > once. > > > > If we then need an extra Get, we can create a separate getter-only > > > > TraceEventAgent* GetInstance(). > > > > The reason I am advocating about this is because in the past we have been > > > quite > > > > bitten by the GetInstance() model that lazily brings up the > > MemoryDumpManager > > > / > > > > TracingController / TraceLog. Essentially today there are a bounch of places > > > > that invoke that and is impossible to easily understand in which order they > > > are > > > > created. In fact depending on the combination of --enable-heap-profiling and > > > > --startup-trace that you use they get initialized in different order > > (because > > > > the GetInstance() get called in a different sequence) and that has been a > > huge > > > > mainteinance burdern. > > > > > > > > Hence my suggestion of making it clear when the instance is created (and > > > > returned, that is fine) vs just accessed.. > > > > > > But, this is not guaranteed to be called once. This will be called in the > > start > > > of services and if we bundle several services in one process, this will be > > > called several times, until we have a better way of initializing these > > > one-per-process libs in the service manager > > > > Okay I think I am still missing this point. Realistically, even before we have a > > way to initialize this in the service manager, I expect this to be called only > > from child_thread::main. > > Is there a realistic case where we risk to initialize this more than once? If > > not we should just have a CreateInstance(...) which has a DCHECK against > > double-initialization, and revisit that if we have an actual use case. > > Agree, let's differ this discussion to the CL that needs this. For now, I just > add a GetInstance() method. Creator of this lib, e.g. ChildThreadImpl, should > manage its lifetime, which should be just storing it in an internal > std::unique_ptr variable. There is a DCHECK that verifies no more than one > instance is live at any time in each process. > > https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... > File services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc (right): > > https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... > services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:50: metadata_generator_functions_.push_back(generator); > On 2017/05/16 21:19:05, oystein wrote: > > Would a DCHECK(recorder_); make sense here? > > Not really. Recorders are given at StartTracing time and are destroyed after flushing events is complete. Metadata generator functions are added at the initialisation time only. The generators are then called every time tracing is stopped after flushing all trace events (please see OnTraceLogFlush), at which time we must have a recorder (but not necessarily here). Acknowledged. > > https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... > services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:70: base::trace_event::TraceLog::GetInstance()->Flush(base::Bind( > On 2017/05/16 21:19:06, oystein wrote: > > This can get called from ~ChromeTraceEventAgent(), passing > > base::Unretained(this) to a callback seems bad. Even when it's not called from > > the destructor, couldn't this class be destructed by the messagepipe being > > closed at any time? > > Good point. Changed the destructor to not calling this anymore. > > The lifetime of this object is not related to the state of the internal variable binding_. mojo::Binding does not take ownership of the implementation (i.e. ChromeTraceEventAgent); mojo::StrongBinding does. > > In the use cases that I have in mind, this client library is created close to when a process (e.g. a renderer) is created and is destroyed when the process is destroyed (of course, tests are exceptions). So, if we ignore the shutdown sequence at the very end of a process life, the ChromeTraceEventAgent instance should live as long as the TraceLog instance lives. That's fair, though it would be great if we could come up with a DCHECK or something which would let us know if this ever changes, so it doesn't come back to bite us at some point. Would it be feasible to DCHECK if TraceLog::GetInstance()->flush_output_callback_ is currently pointing to 'this' in ~ChromeTraceEventAgent()? > https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... > services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:82: auto status = base::trace_event::TraceLog::GetInstance()->GetStatus(); > On 2017/05/16 21:19:05, oystein wrote: > > nit: Since you're referring to fields of "status" below, rather than just using > > it opaquely, I would specify the type rather than using auto. > > Done. > > https://codereview.chromium.org/2878533003/diff/80001/services/resource_coord... > services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc:103: recorder_->AddMetadata(std::move(metadata)); > On 2017/05/16 21:19:05, oystein wrote: > > Any chance we could add the metadata first? (Basically using a flag to only run > > through the generator functions once). There's situations where it's > > advantageous to be able to pull out the metadata from a large trace file without > > needing to read/decompress the entire file (plus we'd know more about truncated > > files in, say, Slow Reports). > > The agent can write the metadata first; but the difficulty is that the tracing > service should wait for all children to send metadata before it can write metadata > to the sink (a trace file for example). But, it can start writing trace events as > soon as at least one of the children starts sending trace events. > > So, if we want to write the metadata first, we have to wait for all children to > reply and buffer all trace events that we have received so far so that we can > write them to the output stream after writing the metadata which will not be > very efficient memory wise and CPU wise. > > An alternative way is to somehow mark or hard-code children that we expect > to receive metadata from them (the browser maybe?) and just wait for them. > > I changed this to write the metadata first. I will add a section to discuss > this in the design doc too. We can make a decision on the CL that implements > the coordinator to whether write the metadata to the sink first or start > writing events as soon as possible. That makes sense to me; and doing this just on a per-agent basis rather than globally seems fine for now, it isn't something we should invest a bunch of time in. Thanks!
Still lgtm. https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:60: interface Coordinator { On 2017/05/17 21:33:17, chiniforooshan wrote: > On 2017/05/17 21:24:02, kenrb wrote: > > How do you prevent renderer processes from being able to connect to this > > interface? I had a quick look through the prototype and couldn't see anything > -- > > is the intent to add a ConnectionFilter? > > Isn't it enough that we don't "provide" the Coordinator interface > to the renderer in the tracing service manifest or in > content/public/app/mojo/content_browser_manifest.json and only > provide the AgentRegistry interface? Yes, thanks, this is right, sorry about my confusion. More specifically, the renderer manifest doesn't require the tracing capability, which is what the service manager will check before allowing an interface to be bound.
Thanks everyone! > That's fair, though it would be great if we could come up > with a DCHECK or something which would let us know if this > ever changes, so it doesn't come back to bite us at some > point. Would it be feasible to > DCHECK if TraceLog::GetInstance()->flush_output_callback_ > is currently pointing to 'this' in ~ChromeTraceEventAgent()? Woah, wouldn't that be cool :) Unfortunately, bind_state_ is a private member of a base::Callback; otherwise we could look at the first bound argument of bind_state_ of the callback. I added an boolean to track when we set the callback and when we are done with it, as a workaround. Then, I DCHECK that boolean in the destructor. https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... File services/resource_coordinator/public/interfaces/tracing/tracing.mojom (right): https://codereview.chromium.org/2878533003/diff/100001/services/resource_coor... services/resource_coordinator/public/interfaces/tracing/tracing.mojom:60: interface Coordinator { On 2017/05/18 02:19:52, kenrb wrote: > On 2017/05/17 21:33:17, chiniforooshan wrote: > > On 2017/05/17 21:24:02, kenrb wrote: > > > How do you prevent renderer processes from being able to connect to this > > > interface? I had a quick look through the prototype and couldn't see > anything > > -- > > > is the intent to add a ConnectionFilter? > > > > Isn't it enough that we don't "provide" the Coordinator interface > > to the renderer in the tracing service manifest or in > > content/public/app/mojo/content_browser_manifest.json and only > > provide the AgentRegistry interface? > > Yes, thanks, this is right, sorry about my confusion. More specifically, the > renderer manifest doesn't require the tracing capability, which is what the > service manager will check before allowing an interface to be bound. Acknowledged.
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, kenrb@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2878533003/#ps140001 (title: "review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, oysteine@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2878533003/#ps160001 (title: "fix a missing dep")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, oysteine@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2878533003/#ps180001 (title: "use thread checker macros")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495131380411320, "parent_rev": "1df3575f49c762037f3c3af6451a4b9e0e79ac88", "commit_rev": "b3cad27b421b2aa9f6b0a89c80238480ba14023e"}
Message was sent while issue was closed.
Description was changed from ========== tracing: The client lib of the tracing service Any process that needs to connect to the tracing service and provide trace events can just call TraceEventAgent::GetOrCreateInstance and pass a connection to an implementation of the AgentSet interface. A somewhat working prototype that includes the service implementation: https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 ========== to ========== tracing: The client lib of the tracing service Any process that needs to connect to the tracing service and provide trace events can just call TraceEventAgent::GetOrCreateInstance and pass a connection to an implementation of the AgentSet interface. A somewhat working prototype that includes the service implementation: https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 Review-Url: https://codereview.chromium.org/2878533003 Cr-Commit-Position: refs/heads/master@{#472989} Committed: https://chromium.googlesource.com/chromium/src/+/b3cad27b421b2aa9f6b0a89c8023... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b3cad27b421b2aa9f6b0a89c8023... |