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

chore: using go-kit fields plus minor improvements #5

Merged
merged 22 commits into from
Nov 17, 2023

Conversation

fracasula
Copy link
Collaborator

@fracasula fracasula commented Nov 10, 2023

Changes

  • labels for go now use the go-kit type which allows for both sugared and non-sugared logging
  • split main.go to always close files on error
  • renamed team to domain
    • domain is more specific and a single team can have multiple domains (e.g. there is no warehouse team, there is the pipelines team with warehouse domain)
  • adding some helper functions to the go template
    • the goal here is to jump less from the template file to types.go. ideally we want to be able to understand the majority of what is going on just by looking at the template file and then leave only functions that are too complex for a template to types.go

Related PRs

For a showcase about how this can be used check the Pull Requests below:

@fracasula fracasula changed the title Feature/pipe 511 observability kit changes chore: using go-kit fields plus minor improvements Nov 10, 2023
koladilip
koladilip previously approved these changes Nov 11, 2023
Copy link
Collaborator

@koladilip koladilip left a comment

Choose a reason for hiding this comment

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

Looks good, I am thinking rudder-go-kit should depend on obs-kit, should we move the functionality related to observability to obs kit?

@fracasula fracasula marked this pull request as ready for review November 15, 2023 13:52
@lvrach
Copy link
Member

lvrach commented Nov 15, 2023

Shall we introduce a github action ensuring the generators have been executed, something like this

cmd/generate/labels.yaml Outdated Show resolved Hide resolved
lvrach
lvrach previously approved these changes Nov 15, 2023
var (
UploadId = name[int]("uploadId")
UploadTime = name[Time]("uploadTime")
Copy link
Member

Choose a reason for hiding this comment

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

was this removed intentionally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the old name[T] was replaced with the gokit/logger.Field.
If you're referring to UploadTime, I want to tackle the fields that we'll need in separate PRs. Here we're just putting down some ground work then we can do a dedicated PR where we just add stuff to the labels.yaml file and generate.

cmd/generate/main_test.go Outdated Show resolved Hide resolved
cmd/generate/main_test.go Outdated Show resolved Hide resolved
var (
UploadId = name[int]("uploadId")
UploadTime = name[Time]("uploadTime")
UploadID = func(v int64) log.Field { return log.NewIntField("uploadId", v) }
Copy link
Member

Choose a reason for hiding this comment

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

[non-blocking] Should we introduce a prefix for domain-specific metrics ? or maybe put them in a different package ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We thought about it but avoided it to cause a conflict on purpose to make people rethink the label name. Perhaps if it's a conflict it might be a duplicate. If it's not a duplicate then a prefix can be added manually by changing the label name itself (i.e. by making the label name more verbose/explanatory for example).

@fracasula
Copy link
Collaborator Author

Shall we introduce a github action ensuring the generators have been executed, something like this

@lvrach Done. I pushed a label without generating and the CI broke here: https://github.com/rudderlabs/rudder-observability-kit/actions/runs/6879882237/job/18712820643?pr=5

@fracasula
Copy link
Collaborator Author

fracasula commented Nov 15, 2023

@lvrach @koladilip I added also linters and checkers that linting, go mod tidy and formatting are done properly along with tests running on CI.

Since you both approved I'm going to merge as soon as I have a rudder-go-kit version even if the approvals got dismissed (we'll change the setting later).

@fracasula fracasula added the DO NOT MERGE Approved but need to check more label Nov 15, 2023
@fracasula
Copy link
Collaborator Author

DO NOT MERGE: we need a stable version of the go-kit first. Please review: rudderlabs/rudder-go-kit#202

@fracasula fracasula changed the title chore: using go-kit fields plus minor improvements DO NOT MERGE chore: using go-kit fields plus minor improvements Nov 15, 2023
@fracasula fracasula removed the DO NOT MERGE Approved but need to check more label Nov 17, 2023
@fracasula fracasula changed the title DO NOT MERGE chore: using go-kit fields plus minor improvements chore: using go-kit fields plus minor improvements Nov 17, 2023
@fracasula fracasula merged commit ccfb916 into main Nov 17, 2023
3 checks passed
@fracasula fracasula deleted the feature/pipe-511-observability-kit-changes branch November 17, 2023 16:05
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.

3 participants