Cleanup API for consistency, make it more consumable (breaking changes) #213

jotak · 2022-05-31T13:19:56Z

More consistent naming (camel-case) cf There are some inconsistencies in the API #206
Add json bindings
New pipeline builder to make it easier to consume and reuse API model
Remove garbage files (?) that prevents importing as lib from another
project

Summary of renamings:

prom.metrics.valuekey => prom.metrics.valueKey
prom.expirytime => prom.expiryTime
kafka.addr => kafka.address
kafka.groupbalancers => kafka.groupBalancers
kafka.startoffset => kafka.startOffset
kafka.batchreadtimeout => kafka.batchReadTimeout
grpc.buffer_length => grpc.bufferLength
network.kubeconfigpath => network.kubeConfigPath
network.servicesfile => network.servicesFile
network.protocolsfile => network.protocolsFile
aggregates.Name => aggregates.name
aggregates.By => aggregates.by
aggregates.Operation => aggregates.operation
aggregates.RecordKey => aggregates.recordKey
aggregates.TopK => aggregates.topK

codecov-commenter · 2022-05-31T13:27:31Z

Codecov Report

Merging #213 (c7c1dab) into main (3d41231) will increase coverage by 0.51%.
The diff coverage is 82.17%.

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   59.32%   59.84%   +0.51%     
==========================================
  Files          59       60       +1     
  Lines        3469     3551      +82     
==========================================
+ Hits         2058     2125      +67     
- Misses       1278     1292      +14     
- Partials      133      134       +1

Flag	Coverage Δ
unittests	`59.84% <82.17%> (+0.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/api/encode_kafka.go	`100.00% <ø> (ø)`
pkg/api/encode_prom.go	`100.00% <ø> (ø)`
pkg/api/transform_filter.go	`100.00% <ø> (ø)`
pkg/api/transform_generic.go	`0.00% <ø> (ø)`
pkg/api/transform_network.go	`100.00% <ø> (ø)`
pkg/api/write_loki.go	`55.55% <ø> (ø)`
pkg/config/config.go	`50.00% <ø> (ø)`
pkg/pipeline/ingest/ingest_collector.go	`52.38% <0.00%> (-0.85%)`	⬇️
pkg/pipeline/transform/transform.go	`100.00% <ø> (ø)`
pkg/pipeline/write/write_loki.go	`64.24% <33.33%> (-1.30%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d41231...c7c1dab. Read the comment docs.

eranra · 2022-05-31T13:40:52Z

pkg/api/api.go

-const TagDoc = "doc"
-const TagEnum = "enum"
+const (
+	FileType              = "file"


@jotak do you see a way to move all those constantly into the "enum" mechanism ... doing so will allow us to have automatic documentation of the specific options and make sure we maintain the list going forward.

Context: I was thinking going forward that we will be able to totally deprecate the free-text from the readme and relay on generated docs from the code ( using the doc tags in structs).

not done yet, it seems to be a bigger refactoring
Currently only the api package is covered by doc, right? The idea would be to cover config as well? (config is like a superset on api) Also I'm not sure how the enum codegen works, is it run on a specific package or on all sources? Again, it's because these types are in config and not in api

eranra · 2022-05-31T13:42:50Z

Makefile

@@ -102,6 +102,10 @@ clean: ## Clean
 tests-unit: validate_go ## Unit tests
 	go test -p 1 -race -coverpkg=./... -covermode=atomic -coverprofile=/tmp/coverage.out $$(go list ./... | grep -v /e2e)

+.PHONY: tests-fast
+tests-fast: validate_go ## Fast unit tests (no race test / coverage)


Can you merge this with the tests-unit stage

maybe by making the -race -coverpkg=./... -covermode=atomic -coverprofile=/tmp/coverage.out a variable the is set in tests-unit and is not set in tests-fast ???

- More consistent naming (camel-case) cf netobserv#206 - Add json bindings - New pipeline builder to make it easier to consume and reuse API model - Remove garbage files (?) that prevents importing as lib from another project

pkg/api/transform_filter.go

KalmanMeth · 2022-06-01T09:38:00Z

pkg/api/transform_filter.go

+	RemoveEntryIfExists      string `yaml:"remove_entry_if_exists" json:"remove_entry_if_exists" doc:"removes the entry if the field exists"`
+	RemoveEntryIfDoesntExist string `yaml:"remove_entry_if_doesnt_exist" json:"remove_entry_if_doesnt_exist" doc:"removes the entry if the field doesnt exist"`


In other locations, we use camelCase for the yaml and json entries.

so, I didn't change the enums because they are JSON values, not JSON keys. I think it's fine to keep them as they are, no?
so changes are only focusing on keys

KalmanMeth · 2022-06-01T09:39:28Z

pkg/api/transform_network.go

+	ConnTracking  string `yaml:"conn_tracking" json:"conn_tracking" doc:"set output field to value of parameters field only for new flows by matching template in input field"`
+	AddRegExIf    string `yaml:"add_regex_if" json:"add_regex_if" doc:"add output field if input field satisfies regex pattern from parameters field"`
+	AddIf         string `yaml:"add_if" json:"add_if" doc:"add output field if input field satisfies criteria from parameters field"`
+	AddSubnet     string `yaml:"add_subnet" json:"add_subnet" doc:"add output subnet field from input field and prefix length from parameters field"`
+	AddLocation   string `yaml:"add_location" json:"add_location" doc:"add output location fields from input"`
+	AddService    string `yaml:"add_service" json:"add_service" doc:"add output network service field from input port and parameters protocol field"`
+	AddKubernetes string `yaml:"add_kubernetes" json:"add_kubernetes" doc:"add output kubernetes fields from input"`


Elsewhere we use camelCase for the yaml and json definitions.

same explanation here, it's because it's an enum, these names are values

pkg/api/transform_filter.go

ronensc · 2022-06-01T08:05:54Z

pkg/api/transform_generic.go

 }

 type TransformGenericOperationEnum struct {
-	PreserveOriginalKeys string `yaml:"preserve_original_keys" doc:"adds new keys in addition to existing keys (default)"`
-	ReplaceKeys          string `yaml:"replace_keys" doc:"removes all old keys and uses only the new keys"`
+	PreserveOriginalKeys string `yaml:"preserve_original_keys" json:"preserveOriginalKeys" doc:"adds new keys in addition to existing keys (default)"`


Is the different format between yaml and json intended?
The json tag in the other enums are formatted in snake_case.
Do we want to stick with camelCase also with the enum values?

oops good catch for the json here, they should be the same as yaml.
I'd stick with the snake_case here. My rationale is that it's typical to enforce case on data keys, but not on data values. I mean, values can be pretty much anything we don't have to enforce a style on them ... and currently all enums are snake case, so they are consistent ... sounds good?

I initially suggested camelCase to follows k8s yamls conventions. They apply camelCase also on enum values. For example: imagePullPolicy: IfNotPresent.
Anyway, we can do whatever we want and I'm not against snake_case.

Hmm so in this kube example, first letter is capital. So we'd have PreserveOriginalKeys here. is it what we want? (I don't have a strong opinion)

Oh right. I can't believe I gave an example that contradicts my claim 🤣
So, maybe we should keep our enums as they are for now as you suggest...

ronensc · 2022-06-01T09:28:36Z

pkg/config/pipeline_builder_test.go

+	"github.com/netobserv/flowlogs-pipeline/pkg/api"
+	"github.com/stretchr/testify/require"
+)
+


Should we have a test for NewGRPCPipeline() ?

I haven't tested extensively all possible stages, but yes I can add one

ronensc · 2022-06-01T09:36:41Z

pkg/config/pipeline_builder_test.go

+	require.Equal(t, `{"name":"prom","encode":{"type":"prom","prom":{"metrics":[{"name":"connections_per_source_as","type":"counter","filter":{"key":"name","value":"src_as_connection_count"},"valueKey":"recent_count","labels":["by","aggregate"],"buckets":[]}],"port":9090,"prefix":"flp_","expiryTime":0}}}`, string(b))
+}
+
+func TestForkPipeline(t *testing.T) {


Why is this called ...Fork...? I can't find the fork 😅

it's because both WriteLoki and WriteStdout are called from the same plFork instance, so they take the same follows stage, resulting in a fork. If I wanted to chain WriteLoki and WriteStdout (ok, even if it doesn't make sense and would produce an error at runtime) I would have written:

next := plFork.WriteLoki("loki", api.GetWriteLokiDefaults()) next.WriteStdout("stdout", api.WriteStdout{})

I guess I should add some doc

I see. thanks!

ronensc · 2022-06-01T09:41:57Z

pkg/config/pipeline_builder.go

+func (b *PipelineBuilder) next(name string, param StageParam) PipelineBuilder {
+	b.pipeline.stages = append(b.pipeline.stages, Stage{Name: name, Follows: b.lastStage})
+	b.pipeline.config = append(b.pipeline.config, param)
+	return PipelineBuilder{pipeline: b.pipeline, lastStage: name}


Should we create a new PipelineBuilder or return b?

returning a new PipelineBuilder allows the caller to either chain the next stage (using that new PipelineBuilder) or create a fork by keep using the old PipelineBuilder.
I can rename to PipelineBuilderStage if it helps clarify

jotak · 2022-06-01T12:50:25Z

@ronensc comments addressed, some documentation added

jotak requested review from eranra, KalmanMeth and ronensc May 31, 2022 13:19

eranra reviewed May 31, 2022

View reviewed changes

eranra approved these changes May 31, 2022

View reviewed changes

eranra reviewed May 31, 2022

View reviewed changes

jotak added 2 commits May 31, 2022 17:09

make target tests-fast uses tests-unit

277bc34

jotak force-pushed the api-cleanup branch from bccd239 to 277bc34 Compare May 31, 2022 15:29

Fix write loki with unset ClientConfig

5ceeb2e

jotak force-pushed the api-cleanup branch from 0129526 to 5ceeb2e Compare May 31, 2022 17:09

jotak mentioned this pull request May 31, 2022

Use new FLP API to build pipeline netobserv/network-observability-operator#110

Merged

KalmanMeth reviewed Jun 1, 2022

View reviewed changes

pkg/api/transform_filter.go Outdated Show resolved Hide resolved

KalmanMeth reviewed Jun 1, 2022

View reviewed changes

Fix malformed json/doc annotation

cd46af3

KalmanMeth approved these changes Jun 1, 2022

View reviewed changes

ronensc approved these changes Jun 1, 2022

View reviewed changes

Fix TransformGenericOperationEnum annotations, add test, add doc

c7c1dab

ronensc approved these changes Jun 1, 2022

View reviewed changes

jotak merged commit b7e7bdb into netobserv:main Jun 2, 2022

This was referenced Jun 2, 2022

There are some inconsistencies in the API #206

Closed

Make syntax of config yaml file entries consistent #197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup API for consistency, make it more consumable (breaking changes) #213

Cleanup API for consistency, make it more consumable (breaking changes) #213

jotak commented May 31, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading

eranra May 31, 2022

jotak May 31, 2022

eranra May 31, 2022

jotak May 31, 2022

KalmanMeth Jun 1, 2022

jotak Jun 1, 2022

KalmanMeth Jun 1, 2022

jotak Jun 1, 2022

ronensc Jun 1, 2022

jotak Jun 1, 2022

ronensc Jun 1, 2022

jotak Jun 1, 2022

ronensc Jun 1, 2022

ronensc Jun 1, 2022

jotak Jun 1, 2022

ronensc Jun 1, 2022

jotak Jun 1, 2022

ronensc Jun 1, 2022

ronensc Jun 1, 2022

jotak Jun 1, 2022

jotak commented Jun 1, 2022

		RemoveEntryIfExists string `yaml:"remove_entry_if_exists" json:"remove_entry_if_exists" doc:"removes the entry if the field exists"`
		RemoveEntryIfDoesntExist string `yaml:"remove_entry_if_doesnt_exist" json:"remove_entry_if_doesnt_exist" doc:"removes the entry if the field doesnt exist"`

Cleanup API for consistency, make it more consumable (breaking changes) #213

Cleanup API for consistency, make it more consumable (breaking changes) #213

Conversation

jotak commented May 31, 2022 • edited Loading

codecov-commenter commented May 31, 2022 • edited Loading

Codecov Report

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

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

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

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

jotak commented Jun 1, 2022

jotak commented May 31, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading