( )⚙ D4820 logtoprocess: sends the canonical command name to the subprocess

This is an archive of the discontinued Mercurial Phabricator instance.

logtoprocess: sends the canonical command name to the subprocess
ClosedPublic

Authored by lothiraldan on Oct 1 2018, 12:13 PM.

Details

Summary

One of the use-case of logtoprocess is to monitor command duration. With the
current code, we only get whatever command name the user typed (either
abbreviated or aliased).

This makes analytics on the collected data more difficult. Stores the
canonical command name in the request object. Pass the stored canonical name
in the req.ui.log("commandfinish", ...) call as keyword argument to not
break potential string formatting.

Pass the value as the environment variable named LTP_COMMAND to the called
script.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Oct 1 2018, 12:13 PM
yuja added a subscriber: yuja.Oct 3 2018, 9:04 AM

+def extsetup():
+ # this is to get the canonical name of the command: "commit", not "ci"
+ def wrapdispatch(orig, *args, kwargs):
+ encoding.environ.pop("LTP_COMMAND", None)
+ return orig(*args,
kwargs)
+
+ def wrapruncommand(orig, *args, kwargs):
+ encoding.environ["LTP_COMMAND"] = args[2]
+ return orig(*args,
kwargs)
+
+ extensions.wrapfunction(dispatch,'dispatch',wrapdispatch)
+ extensions.wrapfunction(dispatch,'runcommand',wrapruncommand)

It's scary to update the environment variables globally and pass
LTP_COMMAND in to any child processes.

Can you add a proper way to teach the command name to the logtoprocess
extension, by ui.log() for example?

Also, the word LTP isn't used anywhere. It'll need a better name, and
should be documented.

In D4820#73256, @yuja wrote:

+def extsetup():
+ # this is to get the canonical name of the command: "commit", not "ci"
+ def wrapdispatch(orig, *args, kwargs):
+ encoding.environ.pop("LTP_COMMAND", None)
+ return orig(*args,
kwargs)
+
+ def wrapruncommand(orig, *args, kwargs):
+ encoding.environ["LTP_COMMAND"] = args[2]
+ return orig(*args,
kwargs)
+
+ extensions.wrapfunction(dispatch,'dispatch',wrapdispatch)
+ extensions.wrapfunction(dispatch,'runcommand',wrapruncommand)

It's scary to update the environment variables globally and pass
LTP_COMMAND in to any child processes.
Can you add a proper way to teach the command name to the logtoprocess
extension, by ui.log() for example?
Also, the word LTP isn't used anywhere. It'll need a better name, and
should be documented.

I tried to pass it to the ui.log() but didn't find a clean way to transfer it from dispatch._dispatch to dispatch.dispatch. I tried storing it in the ui object but without luck.

Do you think it would be ok to store it in the request object?

yuja added a comment.Oct 4 2018, 9:15 AM
I tried to pass it to the `ui.log()` but didn't find a clean way to transfer it from `dispatch._dispatch` to `dispatch.dispatch`. I tried storing it in the ui object but without luck.
Do you think it would be ok to store it in the `request` object?

If that helps, yes. I think using request object is better than abusing ui,
given the command lifetime is quite the same as the request.

lothiraldan edited the summary of this revision. (Show Details)Oct 16 2018, 11:10 AM
lothiraldan updated this revision to Diff 12161.
yuja added a comment.Oct 18 2018, 9:16 AM
  • a/hgext/logtoprocess.py

+++ b/hgext/logtoprocess.py
@@ -40,6 +40,9 @@
import sys
from mercurial import (
+ dispatch,
+ encoding,
+ extensions,

Unused.

@@ -110,6 +113,12 @@

script = self.config('logtoprocess', event)
if script:
    if msg:

+
+ # extract the canonical command from the passed opts
+ canonical_command = opts.pop("canonical_command", None)
+ if canonical_command is None:
+ canonical_command = ""
+

  1. try to format the log message given the remaining
  2. arguments try:

@@ -134,7 +143,8 @@

    for key, value in opts.iteritems())
env = dict(itertools.chain(procutil.shellenviron().items(),
                           msgpairs, optpairs),
  • EVENT=event, HGPID=str(os.getpid()))

+ EVENT=event, HGPID=str(os.getpid()),
+ LTP_COMMAND=canonical_command)

As I said before, LTP_* doesn't sound great. Can't we simply pass in
canonical_command as OPT_CANONICAL_COMMAND? If we really want to name
it other than OPT_*, please document it.

And I noticed there's no test. Can you add some?

In D4820#77055, @yuja wrote:
  • a/hgext/logtoprocess.py

+++ b/hgext/logtoprocess.py
@@ -40,6 +40,9 @@
import sys
from mercurial import (
+ dispatch,
+ encoding,
+ extensions,

Unused.

@@ -110,6 +113,12 @@

script = self.config('logtoprocess', event)
if script:
    if msg:

+
+ # extract the canonical command from the passed opts
+ canonical_command = opts.pop("canonical_command", None)
+ if canonical_command is None:
+ canonical_command = ""
+

  1. try to format the log message given the remaining
  2. arguments try:

@@ -134,7 +143,8 @@

    for key, value in opts.iteritems())
env = dict(itertools.chain(procutil.shellenviron().items(),
                           msgpairs, optpairs),
  • EVENT=event, HGPID=str(os.getpid()))

+ EVENT=event, HGPID=str(os.getpid()),
+ LTP_COMMAND=canonical_command)

As I said before, LTP_* doesn't sound great. Can't we simply pass in
canonical_command as OPT_CANONICAL_COMMAND? If we really want to name
it other than OPT_*, please document it.

I misunderstood your previous comment, the canonical command is now sent as OPT_CANONICAL_COMMAND, I still need to process it in a dedicated manner and I've detailed why in the comment associated.

And I noticed there's no test. Can you add some?

I have updated the patch with a test and removed the unused imports.

yuja added a comment.Oct 22 2018, 9:28 AM

+ # extract the canonical command from the passed opts.
+ # we need to pop it up from the opts else the msg will try
+ # being formatted using the passed opts.

Oops, that's probably a bug. IIUC, **opts is just ancillary data, and
the msg[0] should always be parameterized by msg[1:]. That's how blackbox
works.

Can you send a separate patch to fix the issue?

In D4820#77397, @yuja wrote:

+ # extract the canonical command from the passed opts.
+ # we need to pop it up from the opts else the msg will try
+ # being formatted using the passed opts.

Oops, that's probably a bug. IIUC, **opts is just ancillary data, and
the msg[0] should always be parameterized by msg[1:]. That's how blackbox
works.
Can you send a separate patch to fix the issue?

Done, I hope the stack was correctly saved because I had to to it through the interface, phabsend didn't worked.

This revision was automatically updated to reflect the committed changes.