Log duplicated activity events

[fimanishi]

What changed?
Added logs to capture duplicated activity events

Why?
There were some cases where started and completed activity events were duplicated, causing the workflows to fail. Couldn't reproduce it locally, so adding logs to try to identify it earlier and debug it.

How did you test it?
Tested locally

Potential risks

Release notes

Documentation Changes

[fimanishi]

We can't compare events directly. Using this struct to dereference attributes that can guarantee the activity uniqueness

[davidporter-id-au]

I'm not sure this will work, equality in maps is being compared by pointer, not by value ( I think, let's confirm I'm not lying). Let's start with a unit test to validate this is working as expected. I think this is will probably not catch duplicates in its current state, unless you squash it into something that's more comparable like a string.

Ensuring that log is called is a bit annoying, so changing the check to just return a bool is one easy solution. eg:

  isDuplicated := e.checkDuplicatedActivityEvents()
  if isDuplicated{
    e.logger.Error("Duplicate activity task event found",
				tag.WorkflowDomainName(e.GetDomainEntry().GetInfo().Name),
				tag.WorkflowID(e.GetExecutionInfo().WorkflowID),
				tag.WorkflowRunID(e.GetExecutionInfo().RunID),
				tag.WorkflowScheduleID(scheduledEventID),
				tag.WorkflowEventType(event.GetEventType().String())
    )

  }
  })

func (e *mutableStateBuilder) checkDuplicatedActivityEvents() bool {
   ... // check if the fields are duplicated 
}

For the duplicated check, my vague memory was that you were looking to check if the scheduledEventID was duplicated? That's just a primitive type, I think that's possible to just do a map check on? I would start there to begin with, though if you want to check other fields, there's a few solutions, but i'm not entirely sure what it means for the attempt to be duplicated, for example.

[davidporter-id-au]

actually seems I am a bit wrong and clearly don't understand map keys well enough

[davidporter-id-au]

yep, you're correct and I'm wrong (https://go.dev/ref/spec#Comparison_operators), (demonstrated here)

Struct types are comparable if all their field types are comparable. Two struct values are equal if their corresponding non-blank field values are equal. The fields are compared in source order, and comparison stops as soon as two field values differ (or all fields have been compared).

So I take back what I said, but would agree with Tim's point about a unit test, both to check and to also catch NPE problems when dereferencing

[timl3136]

nit: could we have some unit test on those cases?

[fimanishi]

Yeah, I'll add that to show how it works