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

OtelFiber: Add Next config setting #633

Merged
merged 13 commits into from
Jun 26, 2023
Merged

Conversation

emanuelef
Copy link
Contributor

@emanuelef emanuelef commented Jun 5, 2023

Just an idea for #632
If this makes sense I can go on with this idea and start testing it.

@emanuelef emanuelef marked this pull request as draft June 5, 2023 12:46
@ReneWerner87
Copy link
Member

Tests and readme adjustments are missing

@ReneWerner87
Copy link
Member

Why not using the skip middleware or a next function like in all other middlewares

@emanuelef
Copy link
Contributor Author

emanuelef commented Jun 5, 2023

Thanks for looking into that.
This approach is ok for what I need but let me know if there's a better way.
I looked at fiberzap and did something similar.

@emanuelef
Copy link
Contributor Author

Where are the other config functions documented ?

@emanuelef emanuelef marked this pull request as ready for review June 5, 2023 15:01
@emanuelef
Copy link
Contributor Author

emanuelef commented Jun 14, 2023

@ReneWerner87 just wanted to ask if this feature is relevant, otherwise I can close the PR.
I'm using this in my fork but might be we have other ways to do that in otelfiber.
Some other go web frameworks have a similar feature for their OTel instrumentation.

@ReneWerner87
Copy link
Member

Sorry i lost the focus on this, will check it in next days and then let you know that the decision is

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jun 15, 2023

first thank you for the effort
your feature works and offers added value

however, it would be better if we follow the lead of all other middlewares and add a config setting for a check method "Next: func(c *fiber.Ctx) bool"

We have this in all other middlewares and the users should be familiar with it.
https://github.com/search?q=repo%3Agofiber%2Ffiber%20Next%3A&type=code
https://docs.gofiber.io/api/middleware/csrf and so on

this would also give us more flexibility, because what if you don't want to use a url, but a parameter, cookie or header to skip this functionality.

i know in the 2 other contrib middlewares(Fiberzap, otelfiber) a skipurl config property was also added
this one i had unfortunately overlooked and i will add improvement reports/tasks to change this as deprecated and add a next method configuration property there as well

So that this concept is found in all middlewares and the consumers see a red trail
@emanuelef
you would help us a lot if you could adjust your pull request accordingly

@emanuelef
Copy link
Contributor Author

@ReneWerner87 Thanks for looking into that. Yes, with Next it would be much better, I'll try to adapt my PR to use that.
Not sure if it should be a separate PR and close this one.

@@ -28,6 +29,12 @@ func (o optionFunc) apply(c *config) {
o(c)
}

func Next(f func(ctx *fiber.Ctx) bool) Option {
Copy link
Contributor Author

@emanuelef emanuelef Jun 16, 2023

Choose a reason for hiding this comment

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

Not sure if to follow the pattern here should be WithNext, it's a bit different to the standard fiber middlewares where we have ConfigDefault

Copy link
Member

Choose a reason for hiding this comment

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

name it withNext, its a shortcut function to work with the defaults

@emanuelef emanuelef changed the title Skip URIs OtelFiber: Add Next config setting Jun 21, 2023
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

LGTM

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jun 22, 2023

@emanuelef can you

  • add the config possiblities to the README.md and maybe some usage examples

https://github.com/gofiber/contrib/blob/main/otelfiber/README.md

like the others
https://github.com/gofiber/contrib/blob/main/opafiber/README.md#config
https://github.com/gofiber/contrib/blob/main/paseto/README.md#config
https://github.com/gofiber/contrib/tree/main/fibersentry#config
....
and maybe a example for the usage

we will use these readme's later in our https://docs.gofiber.io/ and there it is important to have all the information about this packet

this should show up as an issue report in the repository itself or discord channel

because only a well documented software can be well used

@emanuelef
Copy link
Contributor Author

emanuelef commented Jun 24, 2023

I've added two new sections to the README, but I have some questions there.
Most of the example seems to be inspired by the example of another popular go web framework, I was wondering if we want to have something different here. Also the tests are almost exactly the same and are not covering all of the possible configs (WithSpanNameFormatter is not tested).
I would keep the example in the README minimal and point out what is really needed, in my case when I started using otelfiber in my project I ended up looking at the code to find out that I had to use UserContext() to have the trace propagated.

@gaby
Copy link
Member

gaby commented Jun 24, 2023

@emanuelef Totally agree, that could be done in a separate PR so this one can be merged. :-)

@emanuelef emanuelef requested a review from gaby June 25, 2023 06:43
@ReneWerner87 ReneWerner87 merged commit 9cc7ba7 into gofiber:main Jun 26, 2023
@ReneWerner87 ReneWerner87 linked an issue Jun 26, 2023 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 [Feature]: Add ability to ignore health checks in otelfiber
4 participants