Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add context #312

Merged
merged 2 commits into from
Mar 24, 2024
Merged

add context #312

merged 2 commits into from
Mar 24, 2024

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Jan 12, 2024

@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch from 40940f9 to 32ff16a Compare January 12, 2024 23:10
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch from 32ff16a to fe20a39 Compare January 13, 2024 19:33
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch from fe20a39 to efffc37 Compare January 13, 2024 21:38
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here? unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it only served to bridge the gap with context.

Copy link
Collaborator

@topliceanu topliceanu left a comment

Choose a reason for hiding this comment

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

I only looked at the monitoring package.
I'm confused by this change but I don't have context so I't only a "Comment" review.

@@ -36,7 +36,7 @@ func TestKafkaExporter(t *testing.T) {
nodes := []NodeConfig{generateNodeConfig()}
exporter, err := factory.NewExporter(ExporterParams{chainConfig, feedConfig, nodes})
require.NoError(t, err)
envelope, err := generateEnvelope()
envelope, err := generateEnvelope(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused by the introduction of context here. Most of these methods have no concurrency in their implementation - ctx is not needed and won't help.

Take generateEnvelope, it produces a structure filled with random values. https://github.com/smartcontractkit/chainlink-common/pull/312/files#diff-be6ec1c2691cf18a8a73b7ea25c78bab3dc41d5bccde1df3e9626310e849306bR454-R477

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the relevance of concurrency? We are motivated by carrying request scoped values for tracing.

This func calls generateContractConfig, which calls median.StandardOnchainConfigCodec.Encode, which now requires a context.

@jmank88 jmank88 merged commit 92b84cf into main Mar 24, 2024
5 of 9 checks passed
@jmank88 jmank88 deleted the BCF-2887-context-propagation branch March 24, 2024 18:28
jmank88 added a commit that referenced this pull request Mar 26, 2024
jmank88 added a commit that referenced this pull request Mar 26, 2024
jmank88 added a commit that referenced this pull request Mar 26, 2024
jmank88 added a commit that referenced this pull request Mar 27, 2024
ettec pushed a commit that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants