Download Raw Diff

Details

Reviewers

None

Group Reviewers

Commits

rHG106adc261492: logtoprocess: sends the canonical command name to the subprocess

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 1 2018, 12:13 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

+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?

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.

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 = ""
+

try to format the log message given the remaining

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?

lothiraldan updated this revision to Diff 12296.Oct 22 2018, 8:51 AM

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 = ""
+

try to format the log message given the remaining

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.

+ # 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?

lothiraldan updated this revision to Diff 12298.Oct 22 2018, 9:55 AM

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.

lothiraldan updated this revision to Diff 12307.Oct 22 2018, 11:54 AM

Closed by commit rHG106adc261492: logtoprocess: sends the canonical command name to the subprocess (authored by lothiraldan). · Explain WhyOct 22 2018, 11:56 AM

This revision was automatically updated to reflect the committed changes.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	11521		Oct 1 2018, 12:13 PM	★	★
Diff 2	12161		Oct 16 2018, 11:10 AM	★	★
Diff 3	12296		Oct 22 2018, 8:51 AM	★	★
Diff 4	12298		Oct 22 2018, 9:55 AM	★	★
Diff 5	12307		Oct 22 2018, 11:53 AM	★	★
Diff 6	12308	rHG106adc2614922a24adc1e662ea78c83ec19aa4bf	Oct 22 2018, 9:51 AM	★	★

	Status	Author	Revision
	Closed	lothiraldan	D4820 logtoprocess: sends the canonical command name to the subprocess
	Closed	lothiraldan	D5180 logtoprocess: fix message formatting

	from __future__ import absolute_import			from __future__ import absolute_import

	import itertools			import itertools
	import os			import os
	import subprocess			import subprocess
	import sys			import sys

	from mercurial import (			from mercurial import (
				dispatch,
				encoding,
				extensions,
	pycompat,			pycompat,
	)			)

	from mercurial.utils import (			from mercurial.utils import (
	procutil,			procutil,
	)			)

	# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for			# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
	# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should			# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
	# be specifying the version(s) of Mercurial they are tested with, or			# be specifying the version(s) of Mercurial they are tested with, or
	# leave the attribute unspecified.			# leave the attribute unspecified.
	testedwith = 'ships-with-hg-core'			testedwith = 'ships-with-hg-core'

				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)

	def uisetup(ui):			def uisetup(ui):
	if pycompat.iswindows:			if pycompat.iswindows:
	# no fork on Windows, but we can create a detached process			# no fork on Windows, but we can create a detached process
	# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx			# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx
	# No stdlib constant exists for this value			# No stdlib constant exists for this value
	DETACHED_PROCESS = 0x00000008			DETACHED_PROCESS = 0x00000008
	_creationflags = DETACHED_PROCESS \| subprocess.CREATE_NEW_PROCESS_GROUP			_creationflags = DETACHED_PROCESS \| subprocess.CREATE_NEW_PROCESS_GROUP

This is an archive of the discontinued Mercurial Phabricator instance.

logtoprocess: sends the canonical command name to the subprocess
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 11521

hgext/logtoprocess.py

This is an archive of the discontinued Mercurial Phabricator instance.

logtoprocess: sends the canonical command name to the subprocessClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 11521

hgext/logtoprocess.py

logtoprocess: sends the canonical command name to the subprocess
ClosedPublic

Revision Contents
Changeset List