add Name method to ReceiveChannel and SendChannel interface #1538

bentveljanzx · 2024-07-05T00:54:45Z

What was changed

Added Name method to SendChannel and ReceiveChannel interface
Added tests to assert Namereturns back the same given name.

Why?

We have custom wrappers around signalling receive and send calls so that we can attach OpenTelemetry to our signal payload. Since we want our spans to contain the signal name and happen before calling into the underlying channel.Send method, ideally we could simplify our API to not require a redundant sending of the channelName again.
- ie. telemetrytemporal.ChannelSend[T](ctx workflow.Context, channel workflow.SendChannel, channelName string, payload T)

Checklist

Closes #1304

How was this tested:

Added "TestChannelName" in internal/internal_coroutines_test.go which creates named channels and asserts that the Name method returns back the same name you gave it.

Any docs updates needed?

N/A

CLAassistant · 2024-07-05T00:54:50Z

All committers have signed the CLA.

CLAassistant · 2024-07-05T00:54:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Quinn-With-Two-Ns · 2024-07-05T02:17:38Z

internal/workflow.go

@@ -66,8 +66,16 @@ var (
 )

 type (
+	// commonChannel contains methods common to both SendChannel and ReceiveChannel
+	commonChannel interface {
+		// Name returns the name of the channel.


Can we add a few things to the docs:

Specify that for a SignalChannel , Name is the signal name. Some places in Temporal refer to signal "Name" as "Type" so it may not obvious.

Specify that channels created without an explicit name have a name generated by the SDK and they should not assume it is deterministic. Normally all workflow APIs are deterministic, but in this case the channel name would not be.

Added clarity to the documentation as requested

Quinn-With-Two-Ns · 2024-07-05T02:18:03Z

Overall LGTM, just a couple of documentation points.

cretz · 2024-07-08T13:13:05Z

internal/workflow.go

 	// SendChannel is a write only view of the Channel
 	SendChannel interface {
+		commonChannel


I wonder if this should be on the receive side only (send channel only used if user explicitly chooses to break it up, and they can't get things like Len() either even though technically we could provide it).

What is the reason to restrict Name to only receive side channels? A sender could be interested in them name as much as the receiver is.

Just wondering aloud. Can ask the same of Len. Go allows you to get len and cap on both recv-only and send-only channels but we decided Len() should only be on receive. I just wonder if we need to apply whatever that thought process is here. I don't mind if that same thing shouldn't apply here and we put Name() on both (though I think it'd be worth copying the definition twice vs making a new interface).

For our use-case we only use Name for the SendChannel case, so if anything I'd only want it on the SendChannel.

cretz · 2024-07-08T13:13:48Z

internal/workflow.go

 	// SendChannel is a write only view of the Channel
 	SendChannel interface {
+		commonChannel


I wonder if, even though we use aliases which make this harder, inlining the Name() method with the docs is clearer than a shared interface. It's just two places.

Will make the change if there's agreement between @Quinn-With-Two-Ns and yourself :)

One readability benefit of sharing the interface is it'll ensure documentation remains synced over time if it gets modified

Yes I think inlining is better for readability

Updated and inlined. Also made tests explicitly test ReceiveChannel and SendChannel interfaces with Name

cretz · 2024-07-08T13:14:20Z

internal/workflow.go

+		// A Channel created without an explicit name will use a generated name by the SDK and
+		// is not deterministic.


Suggested change

// A Channel created without an explicit name will use a generated name by the SDK and

// is not deterministic.

// A Channel created without an explicit name will use a generated name by the SDK and

// is deterministic.

It is deterministic correct? (built from channel sequence)

It is not given our normal definition for what is deterministic, and I would not want suers to have any expectation of the SDK generated name

Our normal definition is "same during replay" I thought. Definitely no expectation of generated name, but it's still deterministic. Maybe we should just say "generated name by the SDK" without any additional info about determinism.

Even just creating another channel would change the value, we generally don't say creating a channel or switching the order you create channels in not deterministic.

…plicitly tests Name call

bentveljanzx · 2024-07-09T00:27:18Z

If there's anything else outstanding / blocking merge, please let me know :)

Quinn-With-Two-Ns · 2024-07-09T03:14:07Z

Thanks, merged!

bentveljanzx requested a review from a team as a code owner July 5, 2024 00:54

bentveljanzx changed the title ~~add Name method to ReceiveChannel and SendChannel~~ add Name method to ReceiveChannel and SendChannel interface Jul 5, 2024

Quinn-With-Two-Ns reviewed Jul 5, 2024

View reviewed changes

cretz reviewed Jul 8, 2024

View reviewed changes

Quinn-With-Two-Ns approved these changes Jul 8, 2024

View reviewed changes

bentveljanzx added 4 commits July 8, 2024 16:26

add Name method

e8ad662

add tests

33fa287

adjust documentation based on feedback

d1a25da

update use of word channel to be captialized for consistency

eb6ac13

Quinn-With-Two-Ns force-pushed the add-name-method-to-send-and-receive-channel-interface branch from c3bf2b8 to eb6ac13 Compare July 8, 2024 23:26

inline Name method, add tests to ensure ReceiveChannel/SendChannel ex…

6d0f294

…plicitly tests Name call

Quinn-With-Two-Ns merged commit 25f450f into temporalio:master Jul 9, 2024
13 checks passed

bentveljanzx deleted the add-name-method-to-send-and-receive-channel-interface branch December 9, 2024 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Name method to ReceiveChannel and SendChannel interface #1538

add Name method to ReceiveChannel and SendChannel interface #1538

bentveljanzx commented Jul 5, 2024 •

edited

Loading

CLAassistant commented Jul 5, 2024 •

edited

Loading

CLAassistant commented Jul 5, 2024

Quinn-With-Two-Ns Jul 5, 2024

bentveljanzx Jul 8, 2024

Quinn-With-Two-Ns commented Jul 5, 2024

cretz Jul 8, 2024

Quinn-With-Two-Ns Jul 8, 2024

cretz Jul 8, 2024 •

edited

Loading

bentveljanzx Jul 8, 2024

cretz Jul 8, 2024

bentveljanzx Jul 8, 2024

Quinn-With-Two-Ns Jul 8, 2024

bentveljanzx Jul 9, 2024 •

edited

Loading

cretz Jul 8, 2024

Quinn-With-Two-Ns Jul 8, 2024

cretz Jul 8, 2024

Quinn-With-Two-Ns Jul 8, 2024

bentveljanzx commented Jul 9, 2024

Quinn-With-Two-Ns commented Jul 9, 2024

		// A Channel created without an explicit name will use a generated name by the SDK and
		// is not deterministic.

add Name method to ReceiveChannel and SendChannel interface #1538

add Name method to ReceiveChannel and SendChannel interface #1538

Conversation

bentveljanzx commented Jul 5, 2024 • edited Loading

What was changed

Why?

Checklist

CLAassistant commented Jul 5, 2024 • edited Loading

CLAassistant commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentveljanzx Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentveljanzx commented Jul 9, 2024

Quinn-With-Two-Ns commented Jul 9, 2024

bentveljanzx commented Jul 5, 2024 •

edited

Loading

CLAassistant commented Jul 5, 2024 •

edited

Loading

cretz Jul 8, 2024 •

edited

Loading

bentveljanzx Jul 9, 2024 •

edited

Loading