gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
33.21k stars 1.64k forks source link

🐛 [Bug]: Adaptor + otelfiber issue #2641

Closed kperreau closed 9 months ago

kperreau commented 1 year ago

Bug Description

If I combine the Adaptor + Otelfiber contrib Middleware, the method c.BaseURL() return the wrong value. (http:// instead of http://localhost:3000)

How to Reproduce

Steps to reproduce the behavior: Use Adaptor + otelfiber middleware, then call the c.BaseURL() method from an handler.

Expected Behavior

Having the right base url http://localhost:3000 in my case.

Fiber Version

v2.49.2

Code Snippet (optional)

import (
    "github.com/gofiber/contrib/otelfiber"
    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/middleware/adaptor"
    "net/http"
)

func main() {
    app := fiber.New()

    app.Use(otelfiber.Middleware())

    app.Get("/test", func(c *fiber.Ctx) error {
        return c.SendString(c.BaseURL())
    })

    http.ListenAndServe(":3000", adaptor.FiberApp(app))
}

Checklist:

welcome[bot] commented 1 year ago

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

kperreau commented 1 year ago

I add some tests/context here. Basically, call the endpoint http://localhost:3000/test should return the base url http://localhost:3000.

Fiber + Adaptator + Otelfiber

func main() {
    app := fiber.New()

    app.Use(otelfiber.Middleware())

    app.Get("/test", func(c *fiber.Ctx) error {
        return c.SendString(c.BaseURL())
    })

    http.ListenAndServe(":3000", adaptor.FiberApp(app))
}

Result: http://

Fiber

func main() {
    app := fiber.New()

    app.Get("/test", func(c *fiber.Ctx) error {
        return c.SendString(c.BaseURL())
    })

    app.Listen(":3000")
}

Result: http://localhost:3000

Fiber + Otelfiber

func main() {
    app := fiber.New()

    app.Use(otelfiber.Middleware())

    app.Get("/test", func(c *fiber.Ctx) error {
        return c.SendString(c.BaseURL())
    })

    app.Listen(":3000")
}

Result: http://localhost:3000

Fiber + Adaptator

func main() {
    app := fiber.New()

    app.Get("/test", func(c *fiber.Ctx) error {
        return c.SendString(c.BaseURL())
    })

    http.ListenAndServe(":3000", adaptor.FiberApp(app))
}

Result: http://localhost:3000

praem90 commented 11 months ago

The Hostname method on the ctx.go getting called for 3 times for the above example.

The c.fasthttp.Request.URI().Host() returns the correct hostname for the first time but for the further calls returns empty.

Anyone can help me to debug this?

VikasManohar commented 11 months ago

This is my first time trying to contribute, so I may be wrong😃


Using the code snippet given in description for Fiber + Adaptator + Otelfiber, this is the trace:

Following line in adaptor.go sets these values among other things: https://github.com/gofiber/fiber/blob/fa887332189ea9dc0bec8c33be5cefe9e7c817ae/middleware/adaptor/adaptor.go#L69

c.fasthttp.Request.Header.host -> nil
c.fasthttp.Request.uri.host -> 127.0.0.1:3000
c.fasthttp.Request.parsedURI - > true

Following function in fiber.go is called by otelfiber middleware: https://github.com/gofiber/contrib/blob/27b59cf2d203608e9e22a367d275822d3456ae26/otelfiber/fiber.go#L35

func Middleware(opts ...Option) fiber.Handler 

Then the function in this file is called semconv.go : https://github.com/gofiber/contrib/blob/27b59cf2d203608e9e22a367d275822d3456ae26/otelfiber/semconv.go#L32

    - func httpServerTraceAttributesFromRequest(c *fiber.Ctx, cfg config) []attribute.KeyValue
             ...
        semconv.NetHostNameKey.String(utils.CopyString(c.Hostname()))
        semconv.HTTPTargetKey.String(string(utils.CopyBytes(c.Request().RequestURI()))),
            c.fasthttp.Request.parsedURI - > false <--
        semconv.NetHostNameKey.String(utils.CopyString(c.Hostname())),
            - return c.app.getString(c.fasthttp.Request.URI().Host())
            func (req *Request) URI() *URI
                func (req *Request) parseURI() error
                    return req.uri.parse(req.Header.Host(), req.Header.RequestURI(), req.isTLS) <-- this line executes as `c.fasthttp.Request.parsedURI` is set to "false" in the earlier line.

req.uri.parse function resets uri and repopulates uri.host from header.host, but req.Header.Host() is not set when adaptor is used


efectn commented 11 months ago

@VikasManohar you can create a PR. We can use r.URL.Host for the cases that have empty r.Host. However it doesn't seem good idea for non-proxy servers. It's better to check Host header

kperreau commented 9 months ago

Any update on this one?

rharshit82 commented 9 months ago

Hi, I can raise PR for the same. @efectn Can you please assign me this issue?

efectn commented 9 months ago

Hi, I can raise PR for the same. @efectn Can you please assign me this issue?

Done