Skip to content

Allow stan protocol to set native *stan.Msg as a context value#658

Merged
slinkydeveloper merged 1 commit intocloudevents:masterfrom
mercarto:657-stan-message-context
Feb 1, 2021
Merged

Allow stan protocol to set native *stan.Msg as a context value#658
slinkydeveloper merged 1 commit intocloudevents:masterfrom
mercarto:657-stan-message-context

Conversation

@dan-j
Copy link
Copy Markdown
Contributor

@dan-j dan-j commented Jan 25, 2021

See #657

This is an approach which doesn't introduce any breaking changes, if users do nothing their .Receive() functions will receive the same ctx object they always have, however using the new PropogateContext() option will override it.

I'm going to add some comments to other parts of the codebase in this MR because I think I have an alternative approach, but needs discussion

Signed-off-by: dan-j 5727701+dan-j@users.noreply.github.com

dan-j
dan-j commented Jan 25, 2021
View reviewed changes
v2/client/invoker.go Outdated
return fallback
}

return ctx
Copy link
Copy Markdown
Contributor Author

@dan-j dan-j Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes to this function means that if MessageContext.Context() returns nil, then fallback is returned, whereas previously it would return nil.

Not sure if we can do this as it may be a breaking change, however I'd say this is safer since using a nil context is typically a bad idea anyway.

Copy link
Copy Markdown
Member

@slinkydeveloper slinkydeveloper Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change, thus as you outlined before, using another context is a breaking change

@dan-j
Copy link
Copy Markdown
Contributor Author

dan-j commented Jan 25, 2021

An alternative approach, but not sure of the consequences:

Is there a reason why context.Background() is passed in here? If we used ctx instead, then we can get rid of the PropogateContext() option.

sdk-go/v2/binding/to_event.go

Lines 45 to 52 in 93f9fa1

e := event.New()
encoder := (*messageToEventBuilder)(&e)
_, err := DirectWrite(
context.Background(),
message,
encoder,
encoder,
)

@slinkydeveloper
Copy link
Copy Markdown
Member

slinkydeveloper commented Jan 26, 2021

Is there a reason why context.Background() is passed in here? If we used ctx instead, then we can get rid of the PropogateContext() option.

AFAIR I pass context.Background() here to avoid propagating any option related to the writes (ForceBinary, ForceStructured) which will prevent this DirectWrite to work

slinkydeveloper
slinkydeveloper reviewed Jan 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@slinkydeveloper slinkydeveloper left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change, although before merging it, I would love to find out if we can avoid swapping context, but more "decorating it"... I'll try to get back to you with an answer soon

v2/client/invoker.go Outdated
return fallback
}

return ctx
Copy link
Copy Markdown
Member

@slinkydeveloper slinkydeveloper Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change, thus as you outlined before, using another context is a breaking change

@slinkydeveloper
Copy link
Copy Markdown
Member

slinkydeveloper commented Jan 27, 2021 •
edited
Loading

@dan-j Look at #659, the idea of this PR is to reuse the same context, just wrap in it the additional informations we want to propagate from the message up to the client

@slinkydeveloper slinkydeveloper mentioned this pull request Jan 27, 2021
Merged
@slinkydeveloper
Copy link
Copy Markdown
Member

slinkydeveloper commented Jan 27, 2021

I think we should not propagate the whole stan messages, but only the metadata we're interested in. How does it sounds?

@slinkydeveloper slinkydeveloper added this to the SDK 2.4 milestone Jan 27, 2021
@dan-j
Copy link
Copy Markdown
Contributor Author

dan-j commented Jan 29, 2021

I think we should not propagate the whole stan messages, but only the metadata we're interested in. How does it sounds?

Sure, the three things which come to mind are: Redelivered, RedeliveryCount and Sequence.

Do you think these should be standard context options and the getters/setters somewhere in the core SDK, or keep them contained within the stan package? I know protocols such as http won't be able to support this functionality but others may, and since they're values on context consumers can't expect they be set, but can check if they are and act appropriately.

@slinkydeveloper
Copy link
Copy Markdown
Member

slinkydeveloper commented Jan 29, 2021 •
edited
Loading

keep them contained within the stan package?

Let's keep them in the stan package, since they are very protocol specific

@dan-j dan-j force-pushed the 657-stan-message-context branch from d377766 to 6565456 Compare January 31, 2021 13:16
@dan-j
Copy link
Copy Markdown
Contributor Author

dan-j commented Jan 31, 2021

@slinkydeveloper

OK, I've updated this PR to expose a new context decorator which can be used with your new WithInboundContextDecorator() option. This still sets the whole message on the context, but uses an unexported key so the data is only available by using the accessor functions in context.go.

Thoughts?

slinkydeveloper
slinkydeveloper reviewed Feb 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good! Can you add the godocs to the methods?

@dan-j dan-j force-pushed the 657-stan-message-context branch from 6565456 to e3bda64 Compare February 1, 2021 08:24
slinkydeveloper
slinkydeveloper reviewed Feb 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing, can you add a small fancy test for it? Just add it to the actual test an "assert" that MsgMetadata is not empty

@dan-j
Implement context decorator for inbound STAN messages to expose messa…
6fbcac0 
…ge metadata

Signed-off-by: dan-j <5727701+dan-j@users.noreply.github.com>
@dan-j dan-j force-pushed the 657-stan-message-context branch from e3bda64 to 6fbcac0 Compare February 1, 2021 08:57
@dan-j
Copy link
Copy Markdown
Contributor Author

dan-j commented Feb 1, 2021

Added tests with a valid *stan.Message and one where it is not a *stan.Message

@slinkydeveloper slinkydeveloper merged commit d886e31 into cloudevents:master Feb 1, 2021
aneeshkp pushed a commit to aneeshkp/sdk-go that referenced this pull request Mar 1, 2021
@dan-j @aneeshkp
Implement context decorator for inbound STAN messages to expose messa…
2cf4373 
…ge metadata (cloudevents#658)

Signed-off-by: dan-j <5727701+dan-j@users.noreply.github.com>
Signed-off-by: Aneesh Puttur <aneeshputtur@gmail.com>
@renovate renovate bot mentioned this pull request May 21, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@slinkydeveloper slinkydeveloper slinkydeveloper approved these changes

@n3wscott n3wscott n3wscott approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/protocol/nats enhancement New feature or request

Projects

None yet

Milestone

SDK 2.4

Development

Successfully merging this pull request may close these issues.

3 participants

@dan-j @slinkydeveloper @n3wscott