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

Refactor gin middleware #224

Merged
merged 1 commit into from
Dec 9, 2021
Merged

Refactor gin middleware #224

merged 1 commit into from
Dec 9, 2021

Conversation

chimanjain
Copy link
Contributor

Added a New() function which will be used as a standardised function for all the middleware instead of NewMiddleware(It will still be present so we don't break existing bindings).
Updated how we were fetching the route name.

examples/gin/main.go Outdated Show resolved Hide resolved
examples/gin/README.md Outdated Show resolved Hide resolved
examples/gin/README.md Outdated Show resolved Hide resolved
examples/gin/README.md Outdated Show resolved Hide resolved
examples/gin/README.md Outdated Show resolved Hide resolved
examples/gin/main.go Outdated Show resolved Hide resolved
examples/gin/main.go Outdated Show resolved Hide resolved
examples/gin/main.go Outdated Show resolved Hide resolved
examples/gin/main.go Outdated Show resolved Hide resolved
examples/gin/go.mod Show resolved Hide resolved
@chimanjain
Copy link
Contributor Author

PTAL

Copy link
Contributor

@kyrylo kyrylo left a comment

Choose a reason for hiding this comment

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

Looks good. I have a few minor questions/suggestions.

examples/gin/README.md Outdated Show resolved Hide resolved
@chimanjain
Copy link
Contributor Author

PTAL

@@ -2,10 +2,28 @@

This is an example of basic Gin app with Airbrake middleware that reports route stats.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is an example of basic Gin app with Airbrake middleware that reports route stats.
This is an example of a basic Gin app with Airbrake middleware that reports performance data (route stats).

var ProjectId int64 = 363389
var ProjectKey string = "baa755018d5e35e07897ee0087fcce9c"
var ProjectId int64 = 105138 // Insert your Project Id here
var ProjectKey string = "fd04e13d806a90f96614ad8e529b2822" // Insert your Project Key here
Copy link
Contributor

Choose a reason for hiding this comment

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

Change many digits of the project key so that it's definitely not valid. Also, change the project id to something like 999999 so that it's more obvious that the user should change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chimanjain before you merge can you replace this ProjectKey with something like you did in the other main.go file:

var ProjectId int64 = 999999                               // Insert your Project Id here
var ProjectKey string = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" // Insert your Project Key here

## How to run API
## How to run Example API

Copy the example in your testing(eg. ginbrake) folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a "testing (e.g. ginbrake) folder"? Does a user really need to copy it? or can they just run it from the example directory?


Copy the example in your testing(eg. ginbrake) folder.

Insert your project ID and project key in main.go file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Insert your project ID and project key in main.go file
Insert your project ID and project key in the `main.go` file. You can find these values on the settings page for your project.


Insert your project ID and project key in main.go file

run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run:
Run the following commands to build and run this example application.


`curl "http://127.0.0.1:3000/weather/{austin/pune/santabarbara}" -H 'api-key: d4b371692d361869183d92d84caa5edb8835cf7d'`

Once you call the API endpoints, check your project's performance dashboard
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Once you call the API endpoints, check your project's performance dashboard
Once you call the API endpoints, view the Airbrake errors and performance dashboards for your project.

)

var ProjectId int64 = 105138 // Insert your Project Id here
var ProjectKey string = "fd04e13d806a90f96614ad8e529b2822" // Insert your Project Key here
Copy link
Contributor

Choose a reason for hiding this comment

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

Change many digits of the project key so that it's definitely not valid. Also, change the project id to something like 999999 so that it's more obvious that the user should change it.

examples/gin/main.go Show resolved Hide resolved
examples/gin/main.go Show resolved Hide resolved
examples/gin/README.md Show resolved Hide resolved
@chimanjain
Copy link
Contributor Author

I will change the readme file.

@chimanjain
Copy link
Contributor Author

PTAL

Copy link
Contributor

@scottsbaldwin scottsbaldwin left a comment

Choose a reason for hiding this comment

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

Just a couple more things I'd suggest that you change before merging.

examples/gin/README.md Show resolved Hide resolved
examples/gin/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kyrylo kyrylo left a comment

Choose a reason for hiding this comment

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

LGTM (looks good to me), but this is still unresolved: https://github.com/airbrake/gobrake/pull/224/files#r764111783

@chimanjain
Copy link
Contributor Author

The sample application of apex/log will be updated in the separate merge request, I am working on removing the custom severity levels and using the levels provided by apex/log

@kyrylo
Copy link
Contributor

kyrylo commented Dec 9, 2021

Ok, maybe we don't need to change it in this PR? :)

Copy link
Contributor

@scottsbaldwin scottsbaldwin left a comment

Choose a reason for hiding this comment

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

LGTM

@chimanjain chimanjain merged commit 045245f into master Dec 9, 2021
@chimanjain chimanjain deleted the gin-middleware-refresh branch December 9, 2021 15:27
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.

3 participants