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

Islam/axm 1660 tackle hapn requests over lambda extension #17

Merged

Conversation

schehata
Copy link
Collaborator

@schehata schehata commented Sep 25, 2023

The extension was crashing when the passed token is empty or invalid, leading to the extension returning error before it can register itself, and that crashes the main function as well.

I Expect the extension to not crash the main function.

The first solution that came to me is to not return an error and keep the extension running, that leads to a panic from a reference to a nil pointer, which is the Axiom client in that case. The end result is me checking Axiom client is not null before calling it, not a smart solution, let me know if you have another idea to write this better.

@schehata schehata self-assigned this Sep 25, 2023
keep the extension running so that it can register itself, but always
check that axiom is not null before using it, otherwise it will panic.
@schehata schehata marked this pull request as ready for review September 27, 2023 14:44
@flbn
Copy link

flbn commented Sep 27, 2023

The first solution that came to me is to not return an error and keep the extension running, that leads to a panic from a reference to a nil pointer, which is the Axiom client in that case. The end result is me checking Axiom client is not null before calling it, not a smart solution, let me know if you have another idea to write this better.

might be easier to extract this into a fn just so we can reuse the logic?

func safelyUseAxiomClient(axiom *flusher.Client, action func(*flusher.Client)) {
    if axiom != nil {
        action(axiom)
    } else {
        Logger.Error("Attempted to use uninitialized Axiom client.")
    }
}

which could look like:

        case <-ctx.Done():
            // Use safelyUseAxiomClient to flush safely
            safelyUseAxiomClient(axiom, func(client *flusher.Client) {
                client.Flush()
            })
            return nil
...

 safelyUseAxiomClient(axiom, func(client *flusher.Client) {
                if client.ShouldFlush() {
                    client.Flush()
                }
            })

@flbn
Copy link

flbn commented Sep 27, 2023

also, this is separate but how do we feel about using a struct instead of this global variable?

like, instead of this:

var (
	runtimeAPI        = os.Getenv("AWS_LAMBDA_RUNTIME_API")
	extensionName     = filepath.Base(os.Args[0])
	isFirstInvocation = true
	runtimeDone       = make(chan struct{})

	// API Port
	logsPort = "8080"

	// Buffering Config
	defaultMaxItems  = 1000
	defaultMaxBytes  = 262144
	defaultTimeoutMS = 1000

	developmentMode = false
	logger          *zap.Logger
)

something more like:

type Config struct {
    RuntimeAPI        string
    ExtensionName     string
    IsFirstInvocation bool
    LogsPort          string
    MaxItems          int
    MaxBytes          int
    TimeoutMS         int
    DevelopmentMode   bool
    Logger            *zap.Logger
    RuntimeDone       chan struct{}
}

var cfg = Config{
    RuntimeAPI:        os.Getenv("AWS_LAMBDA_RUNTIME_API"),
    ExtensionName:     filepath.Base(os.Args[0]),
    IsFirstInvocation: true,
    LogsPort:          "8080",
    MaxItems:          1000,
    MaxBytes:          262144,
    TimeoutMS:         1000,
    DevelopmentMode:   false,
    RuntimeDone:       make(chan struct{}),
}

and then used like

cfg.Logger.Error('ex')

main.go Show resolved Hide resolved
server/server.go Show resolved Hide resolved
@schehata
Copy link
Collaborator Author

The first solution that came to me is to not return an error and keep the extension running, that leads to a panic from a reference to a nil pointer, which is the Axiom client in that case. The end result is me checking Axiom client is not null before calling it, not a smart solution, let me know if you have another idea to write this better.

might be easier to extract this into a fn just so we can reuse the logic?

func safelyUseAxiomClient(axiom *flusher.Client, action func(*flusher.Client)) {
    if axiom != nil {
        action(axiom)
    } else {
        Logger.Error("Attempted to use uninitialized Axiom client.")
    }
}

which could look like:

        case <-ctx.Done():
            // Use safelyUseAxiomClient to flush safely
            safelyUseAxiomClient(axiom, func(client *flusher.Client) {
                client.Flush()
            })
            return nil
...

 safelyUseAxiomClient(axiom, func(client *flusher.Client) {
                if client.ShouldFlush() {
                    client.Flush()
                }
            })

noice, I updated the code to do exactly that

@schehata schehata merged commit 0b03386 into main Sep 27, 2023
4 of 5 checks passed
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.

None yet

2 participants