-
Notifications
You must be signed in to change notification settings - Fork 61
Some features added to live tracing session. #165
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
base: christophebedard/allow-creating-live-tracing-session
Are you sure you want to change the base?
Some features added to live tracing session. #165
Conversation
Signed-off-by: suchetanrs <[email protected]>
* Enable live tracing through --live option * Allow providing a url to send tracing output Signed-off-by: suchetanrs <[email protected]>
|
Thanks for the PR! I'll try to take a look tomorrow. |
Signed-off-by: suchetanrs <[email protected]>
|
I just found out that a job that passed in my older PR now failed. Seems like a bunch of tests are failing :p |
|
Yeah, don't worry, GitHub CI is a bit broken right now! See #160. You can ignore those test failures. I'll fix them once I get some time. |
|
Thanks for your review comments!! |
|
Hey! @christophebedard I've setup two different laptops with ros2 tracing. I can ssh between the laptops and ros communication between them seems to work. However, when I try to connect to the ip address using babeltrace, I always get a connection refused error. In fact, if I try to view the trace output from the same laptop for an ip that's not localhost, I get the same issue. I've built LTTng from source and I'm trying to understand the flow of functions. It seems like the supported protocols from the input url are Please let me know if you have some inputs here. I'll continue to read and understand what's done in LTTng before finishing the PR just so I'm aware of what I'm doing :p |
|
Thanks for looking into it! I think we might have to use the LTTng relay daemon ( Knowing the above, these sections in the LTTng docs make a lot more sense:
If we combine those two use-cases, we basically achieve what we want, i.e., live tracing and sending the data to a remote system. And the URL format makes more sense! Let me know if that makes sense and if you can get it to work knowing this.
You're very courageous! 😁 |
Signed-off-by: suchetanrs <[email protected]>
0c813e3 to
bf892d9
Compare
|
Thanks a lot for your reply @christophebedard !! I went on an LTTng documentation study spree and hence the delay with PR. I've uploaded my study notes here :p I've addressed your review comments but I have a question:
If we do this, maybe we can somehow use the |
Signed-off-by: suchetanrs <[email protected]>
|
Thanks for the changes, I will take a look tomorrow.
Great!
I'll have to take a look at this!
Oh, interesting! It does look like the In that case, then yes we could definitely spawn it similar to |
|
Thanks!! :D I will add spawning the relayd to this as well! |
christophebedard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments, but overall this is looking good
| default=1000000, | ||
| help='TODO (default: %(default)s)') | ||
| const=100000, | ||
| help='Set the live timer interval (default: %(default)s)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does set the live timer interval, but the main use of the --live flag is to create a live tracing session, so this should be something like:
| help='Set the live timer interval (default: %(default)s)') | |
| help='Create a live tracing session. Optionally set the live timer interval (default: %(default)s)') |
(double check the line length and split it over two lines if necessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| parser.add_argument( | ||
| '--live-url', dest='live_url', type=str, | ||
| default='net://localhost', | ||
| help='Set the live tracing URL origin (default: %(default)s)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a note for some improvements later, no need to do anything now:
We should document what this all means (timer interval, URL, etc.), link to the relevant parts of the LTTng docs, provide some examples, etc. This could go in the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, understood 👍
I will raise a PR for the updates in the README after this.
| :param subbuffer_size_kernel: the size of the subbuffers for kernel events (defaults to 32 | ||
| times the usual page size, since there can be way more kernel events than UST events) | ||
| :param live_timer_interval: the time interval at which the data should be flushed from the | ||
| buffer and sent to the LTTng relay. This is in microseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| buffer and sent to the LTTng relay. This is in microseconds. | |
| buffer and sent to the LTTng relay daemon. This is in microseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| the usual page size) | ||
| :param subbuffer_size_kernel: the size of the subbuffers for kernel events (defaults to 32 | ||
| times the usual page size, since there can be way more kernel events than UST events) | ||
| :param live_timer_interval: the time interval at which the data should be flushed from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also say that the tracing session will be in live mode if this value is not None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| times the usual page size, since there can be way more kernel events than UST events) | ||
| :param live_timer_interval: the time interval at which the data should be flushed from the | ||
| buffer and sent to the LTTng relay. This is in microseconds. | ||
| Used only if live_timer_interval is `True`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Used only if live_timer_interval is `True`. | |
| Used only if live_timer_interval is not `None`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| # TODO(christophebedard): do we need to join the | ||
| # base_path with session_name for a live session? | ||
| # We need to return a path, so maybe format it like: | ||
| # "net://localhost/host/$hostname/$session_name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| # TODO(christophebedard): figure out what to provide here as the URL | ||
| # This depends on how we expect users to use live tracing | ||
| # See the documentation for the url param of lttng_create_session_live() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if live_timer_interval is None: | ||
| assert trace_directory == full_session_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value returned by lttng.lttng_init() (trace_directory) should be equivalent to full_live_url if we're in live mode, so we should assert that they are equal too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| live_tracing_url = ( | ||
| 'net://localhost/host/' + socket.gethostname() + '/' + | ||
| session_name | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my other comment, this should probably use live_url instead of net://localhost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| 'net://localhost/host/' + socket.gethostname() + '/' + | ||
| session_name | ||
| ) | ||
| print(f'live trace data will be sent to: {live_tracing_url} on the system running lttng-relayd') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove "on the system running lttng-relayd" but still mention the relay daemon before the :, like:
| print(f'live trace data will be sent to: {live_tracing_url} on the system running lttng-relayd') | |
| print(f'live trace data will be sent to the relay daemon at: {live_tracing_url}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
Hey, I tried a few things about your question in #165 (comment) If we consider the following scenario: The tracing is launched on a machine say At first, I started the relay daemon only using the command On the other hand, if I start the relay daemon on So the bottom line is that I am confused about what should we inform the user when they run live tracing 😂 On the other hand, like you mentioned in #165 (comment), if we use the |
Signed-off-by: suchetanrs <[email protected]>
Signed-off-by: suchetanrs <[email protected]>
Signed-off-by: suchetanrs <[email protected]>
Hello! @christophebedard
The changes in this PR are as follows:
lttngpyfor creating and destroying a live session--liveargument and added the arguments--live-timer-intervaland--live-tracing-url-originstartverb.