Bug 146922 - InteractionEvents should be enhanced to carry a descriptive payload
Summary: InteractionEvents should be enhanced to carry a descriptive payload
Status: RESOLVED WONTFIX
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: dev   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 0.7   Edit
Assignee: Mylyn Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2006-06-13 17:06 EDT by Brian de Alwis CLA
Modified: 2006-11-15 21:28 EST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Brian de Alwis CLA 2006-06-13 17:06:08 EDT
I'm in a situation where I'd like to add additional information to InteractionEvents deltas than just a simple string.  In my use-cases, I'm monitoring tree-viewer branch expansions, and want to dump the contents of the expanded branch.  This is well-structured information.  I see this information as complementing the delta, and not replacing it.

Mik and I have a short discussion.  Mik emphasized that InteractionEvents should be immutable.

1. InteractionEvents take a payload object, which should support some interface depending on the eventual writer.  We provide a guideline that the payload should be immutable, but have no actual way of enforcing this.  The advantage is that the eventual storage format is deferred until necessary (e.g., XML for context-writers, interaction-logs).

2. InteractionEvents take an additional String field, which is assumed to be XML.  Advantage is that the resulting InteractionEvent is definitely immutable.
Comment 1 Mik Kersten CLA 2006-06-13 22:37:25 EDT
Why can't (1) retain immutability?  The payload would simply have to come on the constructor.

Regarding (2), which could be the better approach simply because it is more simple, is there any reason you couldn't use the existing "delta" field for that?
Comment 2 Brian de Alwis CLA 2006-06-14 17:35:35 EDT
> Why can't (1) retain immutability?  The payload would simply have to come on
> the constructor.

If it can take an arbitrary object (and there is no guideline saying such objects should be immutable) then I can add an object which changes over time.  Consider adding a query-result object: at the time added, it probably does encode the results intended.  But when written out, it may have changed.

> Regarding (2), which could be the better approach simply because it is more
> simple, is there any reason you couldn't use the existing "delta" field for
> that?

I had been thinking that the delta field could be a simple summary.  But of course you're right: this could be folded right into the string.  Perhaps we could support something like: any string beginning with "<?xml?>" will have the remainder dumped unencoded.
Comment 3 Mik Kersten CLA 2006-06-15 18:56:41 EDT
Yes, if the payload object were unrestricted we would run into this problem.  In general I believe that InteractionEvents should contain pointers to data and summaries about interaction, and not the actual data.  So a third option is that you make the delta be a reference to another data structure (e.g. CSV file) dumped somewhere.

But I like your last suggestion and for our internal use would consider switching to this format, since the deltas we've used have always had some kind of schema to them.  Are you thinking of submitting a patch?
Comment 4 Brian de Alwis CLA 2006-06-15 19:26:59 EDT
Working on a patch as I type...
Comment 5 Brian de Alwis CLA 2006-06-15 20:23:54 EDT
I've hacked up the InteractionEventLogger to support the "<?xml?>" indicator for any string-bearing field.

But this doesn't play nicely when dealing with the context writers which use an attribute-style form for writing out InteractionEvents:

  <interactionEvent date="..." delta="..." />

Rereading this again, I think Mik's intention was to only support XML-bearing payloads for the delta field (so the delta is always written out as the element's character value?).  But I've actually found it useful to allow the structureHandle to also be an XML-formatted string too.

One solution might be to support any field as appearing as either an attribute or as an explicit sub-element:

  <interactionEvent date="...">
     <delta>....</delta>
     <structureHandle>....</structureHandle>
  </interactionEvent>

I also haven't figured out how to nicely support InteractionEventLogger.readEvent() to distinguish between XML-encoded payloads and these new explicitly-XML payloads.  Testing for the first character being an "<" seems somewhat less than optimal.
Comment 6 Mik Kersten CLA 2006-06-25 17:19:08 EDT
Any changes to this will have to wait until we are done with 0.6/Callisto...
Comment 7 Brian de Alwis CLA 2006-11-13 17:54:32 EST
My apologies for never actually submitting my promised patch.  Although I had a somewhat-workable solution for the InteractionEventLogger, I couldn't quite figure out a nice way of dealing with XML payloads in the SaxContext{Reader,Writer} setup.  So I punted on this PR thinking I'd get back to it later.  And guilt now prompted me to revisit it.  And my suggestion is now to mark this as WONTFIX and to simply ensure that the InteractionEventLogger and SaxContext{Reader,Writer} are content-agnostic and simply read/write suitably encoded strings.

There are two problems, one more conceptual and the other more practical:

(1) How does the XML payload get added to the InteractionEvents in the first place?  If the payload is provided as an XML-encoded string, then why would we expect to have some XML parser do something on read?  We've discussed some of the concerns about immutability if we allow the IE to hold onto objects.  If the objects to be provided are simply summary objects, then there should be no difficulty in provided them as XML-encoded strings.

(2) Assuming we did allow objects, then we need to provide some infrastructure for marshalling/unmarshalling these objects to/from an XML stream.  But we use two different formats between the InteractionEventLogger and SaxContent{Reader,Writer}: defining something that works with both would be painful.  There's also a problem in that the HtmlStreamTokenizer doesn't properly support empty-elements with attributes (e.g., `<foo bar="1"/>').

So given that element attributes have no limit on their size, I'd suggest things remain as they are and that people interested in writing more sophisticated information just provide it as an XML-encoded string, and process it appropriately afterwards.
Comment 8 Mik Kersten CLA 2006-11-14 20:34:34 EST
That sounds right Brian.  Thanks for following up.  Could you add a summary of your suggestions to the following page, mentioning the use case?  Others are likely to come across if they want to log a richer set of information as you did.

http://wiki.eclipse.org/index.php/Mylar_Integrator_Reference#Mylar_Monitor
Comment 9 Brian de Alwis CLA 2006-11-15 14:31:00 EST
Wiki page updated.
Comment 10 Mik Kersten CLA 2006-11-15 21:28:43 EST
Good stuff, thanks!