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

Organize chronicle exporter by http and grpc #2044

Draft
wants to merge 8 commits into
base: release/v1.67.0
Choose a base branch
from

Conversation

djaglowski
Copy link
Member

This PR contains many changes so it may be wise to chunk it out into smaller changes. However, I'm opening it as a draft in order to facilitate feedback on the overall direction.

Changes included:

  • Separate "http exporter" and "grpc exporter".
  • Host metrics collection code is more clearly limited to the grpc exporter.
  • Separate marshaling code & deletes mocking of marshaler. We shouldn't be mocking things that are end-to-end testable in a local environment.
  • Implement mock servers to maximize code coverage. Specifically, this allows us to exercise most of the client code.
  • It's still necessary to mock a few functions, basically limited to building endpoints and bypassing authentication.

Future work:

  • No changes to configuration for the sake of backwards compatibility. However, it's quite clear that having all configuration at the same level in a single struct makes it difficult to logically enforce the rules. We can consider whether to migrate this later.
  • This does not yet include tests for splitting payloads, but we should be able to add them more easily now, since we are not mocking the entire clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but calling this file util_test.go makes me think that it's a test file for util.go, not a util file for test files. test_util.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's fair. It probably doesn't need to be a separate file at anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code ended up only being used only in the http tests, so I moved it there instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a later adjustment, but worth trying to break this out into a GRPC protoMarshaler and an HTTP protoMarshaler? The methods on protoMarshaler are largely only used by one of the two workflows.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're probably right. I'll take a closer look at this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I split the marshaler into a base struct, and separate http and grpc marshalers, and moved them into an internal package to ensure testing is clean.

Comment on lines 49 to 52
customerID, err := uuid.Parse(cfg.CustomerID)
if err != nil {
return nil, fmt.Errorf("parse customer ID: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This happens in both exporters but is only used as a parameter to newProtoMarshaler - if we don't split protoMarshaler into two, worth getting rid of that parameter and just doing the UUID parsing in newProtoMarsheler?

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.

2 participants