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

fix: static metric attributes #918

Closed
wants to merge 1 commit into from

Conversation

stevenh
Copy link

@stevenh stevenh commented Aug 21, 2024

Update dependencies to ensure that static attributes are available.

Fixes: #917

Update dependencies to ensure that static attributes are available.

Fixes: krakend#917
@alombarte
Copy link
Member

Hi @stevenh , thanks for your PR. The commit updates many libraries, some potentially with breaking changes in their functionality, like the router package, azure, AMQP, AWS, JWK validation, CEL, gRPC. We never do massive updates of libraries without testing each one and going deep on the implications of each, so although I understand this is a fix for one of them, I don't think the team will merge this

@stevenh
Copy link
Author

stevenh commented Sep 3, 2024

Interesting, I checked the versions updates and they are all patch or minor, so should not include any breaking changes, if they did break things I would expect them to be caught by tests, allowing it to reported and fixed upstream.

In my experience holding back versions is not only painful for consumers, as they don't get fixes or new functionality but also represents a potential security risk.

Usually this isn't a big issue as consumers can update their own versions, but as plugins must use the exact same versions, this is not possible so vital to keep on top of these for something like krakend. This is exactly what caused me to raise this PR in the first place as we needed the static attribs but couldn't use them, even though its already available in krakend-otel.

I've taken the time to do a review of the module changes and they all seem reasonable, two of the 2 of the 5 are krakend modules, the others are generally minor with the possible exception of gin which has some new features too, but still just 1.9.1 to 1.10.0:

If the preferences is to split the changes out, totally appreciate that, just let me know.

@kpacha
Copy link
Member

kpacha commented Sep 4, 2024

gin 1.10.x breaks the integration tests. I'll approve the workflow execution so it will show

@kpacha
Copy link
Member

kpacha commented Sep 4, 2024

well, the build failed even before running the integration tests. As @alombarte pointed out, dependency upgrades are tricky because not everyone keeps compatibility between minors or even fix versions. That's why we upgrade them one by one and only if there are improvements (better/safer implementations, interesting new features, better resource consumption or performance) or actual fixes. Some times, new versions of our dependencies just extend their features in a direction that does not impact krakend so we don't upgrade these.

@stevenh
Copy link
Author

stevenh commented Sep 4, 2024

Thanks for approving the tests so I can look into the issues.

Out of interest has anyone reported the Gin issue?

@stevenh
Copy link
Author

stevenh commented Sep 7, 2024

Done some digging and found there's few issues:

First off the github.com/uber/jaeger-client-go needs pinning because its depreciated, OpenTelemetry should be used instead, which you're already transitioning to.
replace github.com/uber/jaeger-client-go => github.com/uber/jaeger-client-go v2.28.0+incompatible

Next the content type for negotitate_plain fixture needs updating from application/x-yaml to application/yaml.

Finally https://github.com/krakend/binder needs to be fixed so it correctly cleans up the lua state, PR here

With all that integration tests are passing locally:

go test ./... -race
?       github.com/krakendio/krakend-ce/v2      [no test files]
?       github.com/krakendio/krakend-ce/v2/cmd/krakend-ce       [no test files]
?       github.com/krakendio/krakend-ce/v2/cmd/krakend-integration      [no test files]
ok      github.com/krakendio/krakend-ce/v2/tests        4.795s

I'll get everything updated once the binder fix is merged and a new release created. That will allow this to be used to confirm everything works together when updated before splitting it out.

Hope this helps.

@kpacha
Copy link
Member

kpacha commented Oct 24, 2024

@stevenh I think this PR is getting too convoluted. I'd go for a split so it's easier to test the affected components independently.

regarding the lua/binder update, I've just created a PR: #937

thanks for your contributions!

@kpacha kpacha closed this Oct 24, 2024
@stevenh stevenh deleted the fix/static-metrics branch October 28, 2024 11:10
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.

static metrics attributes not supported
3 participants