600504 - [drain?] Create a unified logging solution

Status: RESOLVED DUPLICATE of bug 563823

(Webtools Graveyard :: Elmo, defect, P5)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems that we're overlapping heavily between Action and LogEntry.

We could go as far as to actually hard copy LogEntry and use it as a common ancestor for Actions in shipping and todo.

Comment 1

[Axel Hecht [:Pike]]

I'd rather not have a dependency on django.contrib.admin in the models, fwiw.

Also, looking at what stas did, it seems tricky to extend the "breed" of a LogEntry. Unless there are docs on how to do it right, of course.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

We'll we can just hardcopy the LogEntry class to our models and inherit from it.

The benefit of such approach will be that we stay closer to django code itself, and we have some rather well thought model we base our log actions on. Whatever we'll do in the future will be easier that way. (any app we'll want to use will rather know LogEntry than our API etc.)

Comment 3

[Axel Hecht [:Pike]]

We should draft out what we need for drain, and see if LogEntry benefits us.

I'm fine with stealing ideas, but we shouldn't cut on features we need/want just because of that.

Comment 4

[Staś Małolepszy :stas]

Pike, Gandalf and I had a chat on IRC about this yesterday. I thought I'd update this bug with the transcript (slightly edited for easier reading).

17:21 <@Pike> I'm not convinced that Action and change notification data need to be going hand in hand, fwiw
17:28 <@Pike> I'm not convinced that drain should be a replacement for Action-like objects
17:28 < gandalf> stas: I think that Pike's Drain concept is more about communication than storing
17:29 < stas> right, so I thought it was both, as a matter of fact
17:39 < stas> Pike: even if drain is more about communication, it seems to me like a good place of a unified logging solution too
17:39 < stas> can you try to look at the code I wrote in this way? :)
17:41 <@Pike> basically, yeah, somewhat like that

Here's the code: http://bitbucket.org/stasm/drain/

Then, Pike had some comments:

17:40 <@Pike> stas: I'll need to wrap my head around the details of generic_relation, but I have a few comments ad-hoc

I'll reply to them inline.

17:41 <@Pike> I'd probably not do the implicit LogEntry code, I'm not sure I favor that. In particular if calling code wants to support both LogEntry and drain independently

This is fixed as of http://bitbucket.org/stasm/drain/changeset/a713e69a2f0f


17:42 <@Pike> Property should be a ManyToMany, with Property objects being unique (like I do for builds, for example)
17:42 <@Pike> I think otherwise that table will get way to bloated
17:42 < stas> Property is manytomany
17:43 < stas> just not called so explicitly
17:44 <@Pike> I see Property with a ForeignKey, so "locale":"af" will not be one property for multiple Actions, AFAICT
17:44 <@Pike> you might be able to re-use the Key only?
17:45 < stas> yeah, right now only the Key
17:45 < stas> ok, I see what you mean
17:45 < stas> Property is the intermediary model between Action and Key
17:45 < stas> but the value of it will get repeated
17:46 < stas> just wonder what the benefit of making this even more many to many is, really
17:47 < stas> 'locale':'af' might be one property, but you'll still need another table to link it to actions, no?
17:47 <@Pike> I guess the value comes when we build tag clouds to show interesting picks to follow/watch
17:48 <@Pike> yeah. It really depends on how flexible the value of a property is in the end
17:49 < stas> good use case with the tag clouds, although I think the current models (in what I wrote today) can get us the same thing. need to think about this. 

The current approach can be seen here: http://bitbucket.org/stasm/drain/src/a713e69a2f0f/models.py. It can be summarized as follows:
  Action --> Property <-- Key
The value of a property is stored on an intermediary model between the Action and the Key (the name) of the property.

If I understand well, what Pike suggests is:
  Action <--> Property
with:
Action:
  properties = ManyToManyField(Property)

Property:
  key = CharField
  value = CharField
  Meta:
    unique_together = (key, value)

I'm fine with that, so if everyone agrees, I'll go ahead and make the change.


17:47 <@Pike> I'm wondering if we should support the message to be templatized, i.e., message being "applabel","id","data"
17:49 <@Pike> re message, we could then store different formats for mail messages, feeds, etc, and update them on errors more easily
17:50 < stas> so message would just be a dict that is a context to render a template in?
17:51 <@Pike> message data would be that, and then a way to select the template in a way that's unique across customers

Pike: why do we need applabel and id in message? We can take them directly from the Action via Action.content_type.app_label and Action.subject_id.


19:29 <@Pike> stas: for when you get back, I'd think that content_object is the same thing as get_edited_object, and I'd probably go for "subject" as name, with subject_type and subject_id (if that doesn't conflict)
20:29 < stas> Pike: re
20:30 < stas> good point about the get_edited_object
20:30 < stas> I put it there before I added the explicit generic fk
20:30 < stas> now it's redundant

Fixed in http://bitbucket.org/stasm/drain/changeset/ec8a84ff834d and http://bitbucket.org/stasm/drain/changeset/bbf595fc14ab. I renamed "content_type" to "subject_content_type". It's longer, but I think that keeping "content_type" in there has value, as it's somewhat of a keyword in django.


20:31 < stas> not sure about the s/object/subject change. from the grammatical point of view, I'm used to calling the object of the action "an object", while the subject is the person performing the action
20:31 < stas> I might be wrong here though
20:38 <@Pike> I'm more thinking about this as Message instead of Action ;-)
20:44 < stas> oh, so more like "I was changed by..." ? :)
20:44 < stas> a... signal? :)
20:47 <@Pike> well, pretty much that it's the subject of the message, less in a grammatical but more in a messaging sense

Agreed. A subject can be "a person or thing that is being discussed, described, or dealt with". Fixed in http://bitbucket.org/stasm/drain/changeset/180d2da8f6ed.


19:30 <@Pike> also, I wonder how to do URLs, view-name and params to be passed to urlvar or plain urls? not sure 

urlvar could work, but I still don't have a good idea on how to make this work for todos.

Comment 5

[Staś Małolepszy :stas]

This might be a dupe of bug 563823.

Comment 6

[Axel Hecht [:Pike]]

Resolving as DUPE of bug 563823.