-
Notifications
You must be signed in to change notification settings - Fork 6
feat: pre-existing app/transaction instance detection #95
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
base: main
Are you sure you want to change the base?
Conversation
…alization in InstrumentMain
…ications in main and functions
parser/agent.go
Outdated
| } | ||
|
|
||
| } | ||
| if assign, ok := cursor.Node().(*dst.AssignStmt); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this block be attempted if the above block fails? Or should a failure of the conditional on 114
if decl, ok := cursor.Node().(*dst.FuncDecl); ok
cause this logic to be skipped?
parser/agent.go
Outdated
| if decl, ok := cursor.Node().(*dst.FuncDecl); ok { | ||
| // Print out return values before caching them | ||
| if decl.Type.Results != nil { | ||
| for _, result := range decl.Type.Results.List { | ||
| // Checking if return type of function is a new relic application | ||
| if starExpr, ok := result.Type.(*dst.StarExpr); ok { | ||
| if ident, ok := starExpr.X.(*dst.Ident); ok { | ||
| if ident.Path == codegen.NewRelicAgentImportPath && ident.Name == "Application" { | ||
| manager.setupFunc = decl | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, this would benefit from de-nesting, where instead of continuing execution on a successful check, we return early/break on an unsuccessful conditional.
…ity and functionality
refactor: de-nest existing app check logic
…gApplicationInMain
…nstrumentation process
| if callExpr, ok := expr.(*dst.CallExpr); ok { | ||
| if selExpr, ok := callExpr.Fun.(*dst.SelectorExpr); ok { | ||
| if selExpr.Sel.Name == "End" { | ||
| // Check if the End method is called directly on the transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add support for defer transactions
parser/preinstrumentationtracing.go
Outdated
| for _, rhs := range stmt.Rhs { | ||
| if callExpr, ok := rhs.(*dst.CallExpr); ok { | ||
| if selExpr, ok := callExpr.Fun.(*dst.SelectorExpr); ok { | ||
| if _, ok := selExpr.X.(*dst.Ident); ok && selExpr.Sel.Name == "StartTransaction" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for net/http we should be checking FromContext and not StartTransaction
| } | ||
|
|
||
| funcCall, ok := ident.Obj.Decl.(*dst.FuncDecl) | ||
| if !ok || manager.setupFunc != funcCall { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handleAssignStmtForAgentVariable function require or assume that a setupFunc has been detected? If manager.setupFunc is nil, should we even ever bother executing this function?
parser/manager.go
Outdated
|
|
||
| instrumentPackages(m, tracingFunctions...) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the current state of the code, and error will never be returned here. Is it expected that this function can never fail? If so, we should remove the expectation of a return value, otherwise we should identify and return possible error states.
…es by removing unused parameters
Co-authored-by: bduranleau-nr <[email protected]>
Co-authored-by: bduranleau-nr <[email protected]>
…cks for nil values
Changes
preinstrumentationDetails
This PR begins our initiative to upgrade Go Easy from a tool that is really helpful for onboarding new users with no existing New Relic instrumentation to a tool that users can use continuously.
Application Detection
checkForExistingApplicationInMain()-> This function checks for existing application in main. If an application is detected in a function inside of main, we mark that one as a setup function and will not conduct tracing on it.Transaction Detection
TransactionCache-> TransactionCache is responsible for tracking existing transactions within a Go application. It maintains the following components:Transaction Detection requires us to introduce a new stage in the Go Easy lifecycle with the addition of a
PreInstrumentationTracingFunctionstage.PreInstrumentationTracingFunctionsdefines a function that is executed before any instrumentation is applied to a code block. These functions are executed on every node in the DST tree of every function declared in an application.It is important to note that these tracing functions will NOT be modifying the existing syntax tree, only scanning it.
With the introduction of the
PreInstrumentationTracingFunctiona new tracing function calledDetectTransactionsis created. DetectTransactions analyzes the AST to identify and track transactions within function declarations. It updates theTransactionCachewith function declarations and expressions related to transactions. Once theTransactionCachecontains the lifespan of a transaction, we can then reference the cache within ourTraceFunction()to skip creating new transactions if there is an active transaction within a function.Sample Code
will cause the TransactionCache to look like
From this, we can determine which sections of code are already instrumented within the TraceFunction