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

[TT-13766] Bump newrelic dependency #6809

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Dec 23, 2024

User description

TT-13766
Summary Bump newrelic dependency
Type Story Story
Status In Code Review
Points N/A
Labels SESAP

This PR bumps the dependency with supposedly minimal changes, following the extensive upgrading guide.

Needs an e2e test with newrelic (see ticket).

https://tyktech.atlassian.net/browse/TT-13766


PR Type

Enhancement, Dependencies


Description

  • Migrated New Relic integration to use the updated v3 library.
  • Introduced a new internal/service/newrelic package for centralized New Relic functionality.
  • Refactored middleware, server, and proxy muxer to use the new context-based transaction handling.
  • Removed the old gateway/newrelic.go file and its outdated implementation.
  • Updated go.mod and go.sum to include the new New Relic v3 library and nrgorilla integration.

Changes walkthrough 📝

Relevant files
Enhancement
middleware.go
Refactor middleware to use updated New Relic context-based API

gateway/middleware.go

  • Replaced direct usage of newrelic.Transaction with a new context-based
    approach using newrelic.Context.
  • Updated middleware logic to use the new StartSegment method from the
    updated New Relic library.
  • Adjusted imports to use the new internal service/newrelic package.
  • +3/-3     
    newrelic.go
    Remove old New Relic setup and instrumentation logic         

    gateway/newrelic.go

  • Removed the old implementation of New Relic setup and instrumentation.
  • Deprecated the file as its functionality has been moved to the new
    internal/service/newrelic package.
  • +0/-100 
    proxy_muxer.go
    Update proxy muxer to use new New Relic context handling 

    gateway/proxy_muxer.go

  • Updated transaction handling to use the new newrelic.Context for
    setting and retrieving transactions.
  • Adjusted imports to use the new service/newrelic package.
  • +7/-2     
    server.go
    Refactor server New Relic setup to use updated library     

    gateway/server.go

  • Refactored SetupNewRelic to use the updated New Relic library and
    configuration options.
  • Updated the global NewRelicApplication variable to use the new
    *newrelic.Application type.
  • Integrated the new service/newrelic package for New Relic
    functionality.
  • +31/-2   
    newrelic.go
    Add new service package for New Relic integration               

    internal/service/newrelic/newrelic.go

  • Introduced a new package for handling New Relic integration.
  • Added context-based transaction management and logging utilities.
  • Implemented a new sink for emitting custom events and metrics to New
    Relic.
  • +99/-0   
    Dependencies
    go.mod
    Update dependencies for New Relic v3 and integrations       

    go.mod

  • Updated New Relic dependency to version 3.
  • Added nrgorilla integration for New Relic.
  • +2/-1     
    go.sum
    Update dependency checksums for New Relic v3                         

    go.sum

  • Updated checksums for the new New Relic v3 and nrgorilla dependencies.

  • +4/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @buger
    Copy link
    Member

    buger commented Dec 23, 2024

    I'm a bot and I 👍 this PR title. 🤖

    Copy link
    Contributor

    github-actions bot commented Dec 23, 2024

    API Changes

    --- prev.txt	2024-12-23 15:17:56.930313340 +0000
    +++ current.txt	2024-12-23 15:17:52.217320866 +0000
    @@ -7669,7 +7669,7 @@
     	ErrEmptyUserIDInSubClaim      = errors.New("found an empty user ID in sub claim")
     )
     var (
    -	NewRelicApplication newrelic.Application
    +	NewRelicApplication *newrelic.Application
     
     	ErrSyncResourceNotKnown = errors.New("unknown resource to sync")
     )
    @@ -7707,9 +7707,6 @@
     FUNCTIONS
     
     func APILoopingName(name string) string
    -func AddNewRelicInstrumentation(app newrelic.Application, r *mux.Router)
    -    AddNewRelicInstrumentation adds NewRelic instrumentation to the router
    -
     func AuthFailed(m TykMiddleware, r *http.Request, token string)
         TODO: move this method to base middleware?
     
    @@ -8696,8 +8693,8 @@
         SetPoliciesByID will update the internal policiesByID map with new policies.
         The key used will be the policy ID.
     
    -func (gw *Gateway) SetupNewRelic() (app newrelic.Application)
    -    SetupNewRelic creates new newrelic.Application instance
    +func (gw *Gateway) SetupNewRelic() (app *newrelic.Application)
    +    SetupNewRelic creates new newrelic.Application instance.
     
     func (gw *Gateway) SignatureVerifier() (goverify.Verifier, error)
         SignatureVerifier returns a verifier to use for validating signatures.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The new New Relic transaction handling logic in createMiddleware might not handle cases where newrelic.Context.Get(r) returns nil properly. Ensure this does not cause runtime issues.

    				if txn := newrelic.Context.Get(r); txn != nil {
    					defer txn.StartSegment(mw.Name()).End()
    Initialization Error Handling

    The SetupNewRelic function logs a warning and skips initialization if an error occurs. Consider whether this is the desired behavior or if the application should fail more explicitly in such cases.

    // SetupNewRelic creates new newrelic.Application instance.
    func (gw *Gateway) SetupNewRelic() (app *newrelic.Application) {
    	var (
    		err      error
    		gwConfig = gw.GetConfig()
    	)
    
    	log := log.WithFields(logrus.Fields{"prefix": "newrelic"})
    	log.Info("Initializing NewRelic...")
    
    	cfg := []newrelic.ConfigOption{
    		newrelic.ConfigAppName(gwConfig.NewRelic.AppName),
    		newrelic.ConfigLicense(gwConfig.NewRelic.LicenseKey),
    		newrelic.ConfigEnabled(gwConfig.NewRelic.AppName != ""),
    		newrelic.ConfigDistributedTracerEnabled(gwConfig.NewRelic.EnableDistributedTracing),
    		newrelic.ConfigLogger(newrelic.NewLogger(log)),
    	}
    
    	if app, err = newrelic.NewApplication(cfg...); err != nil {
    		log.Warn("Error initializing NewRelic, skipping... ", err)
    		return
    	}
    
    	instrument.AddSink(newrelic.NewSink(app))
    	log.Info("NewRelic initialized")
    
    	return
    }
    Custom Event Emission

    The EmitEventErr and EmitComplete methods in the Sink struct use error messages and status strings as part of the custom event name. Ensure this does not lead to excessive cardinality in New Relic metrics.

    func (s *Sink) EmitEventErr(job string, event string, err error, kvs map[string]string) {
    	s.relic.RecordCustomEvent(job+":"+event+":msg:"+err.Error(), makeParams(kvs))
    }
    
    func (s *Sink) EmitTiming(job string, event string, nanoseconds int64, kvs map[string]string) {
    	s.relic.RecordCustomEvent(job+":"+event+":dur(ns):"+strconv.FormatInt(nanoseconds, 10), makeParams(kvs))
    }
    
    func (s *Sink) EmitComplete(job string, status health.CompletionStatus, nanoseconds int64, kvs map[string]string) {
    	s.relic.RecordCustomEvent(job+":health:"+status.String()+":dur(ns):"+strconv.FormatInt(nanoseconds, 10), makeParams(kvs))

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Safeguard against potential issues with SetWebResponse by checking its return value

    Validate that txn.SetWebResponse(w) does not return an error or unexpected behavior,
    as it may alter the response writer in unintended ways.

    gateway/proxy_muxer.go [101]

    -w = txn.SetWebResponse(w)
    +if newWriter := txn.SetWebResponse(w); newWriter != nil {
    +    w = newWriter
    +}
    Suggestion importance[1-10]: 8

    Why: This suggestion adds a safeguard to ensure that the return value of SetWebResponse is valid before assigning it to w. This enhances the robustness of the code and prevents potential issues with altered response writers, making it a valuable improvement.

    8
    Add validation to prevent runtime errors when processing kvs in makeParams

    Ensure that makeParams properly handles cases where kvs contains invalid or
    unexpected data types to avoid runtime panics.

    internal/service/newrelic/newrelic.go [94-96]

     for k, v := range kvs {
    -    params[k] = v
    +    if v != "" {
    +        params[k] = v
    +    }
     }
    Suggestion importance[1-10]: 5

    Why: The suggestion adds a basic validation to skip empty values in the kvs map, which could prevent potential runtime issues. However, it does not address all possible invalid or unexpected data types, limiting its impact.

    5
    Possible issue
    Ensure proper cleanup of New Relic segments by wrapping them in a defer statement at the start of the function

    Ensure that the txn.StartSegment(mw.Name()).End() is executed even if an error
    occurs in the middleware logic by wrapping it in a defer statement at the beginning
    of the function.

    gateway/middleware.go [141-142]

     if txn := newrelic.Context.Get(r); txn != nil {
    -    defer txn.StartSegment(mw.Name()).End()
    +    segment := txn.StartSegment(mw.Name())
    +    defer segment.End()
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the clarity and safety of the code by explicitly assigning the segment to a variable before deferring its End method. This ensures proper cleanup and aligns with best practices for using defer. The improvement is relevant and accurate.

    7
    Improve error handling when initializing the New Relic application to avoid proceeding with an invalid state

    Handle the case where newrelic.NewApplication(cfg...) fails to initialize the
    application, ensuring the program doesn't proceed with an uninitialized New Relic
    application.

    gateway/server.go [277-279]

     if app, err = newrelic.NewApplication(cfg...); err != nil {
    -    log.Warn("Error initializing NewRelic, skipping... ", err)
    -    return
    +    log.Error("Failed to initialize NewRelic: ", err)
    +    return nil
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion improves error handling by logging the error as a failure and returning nil explicitly, which makes the code more robust. However, the improvement is minor since the existing code already logs the error and skips initialization.

    6

    Copy link

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    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.

    2 participants