-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feat/track metrics #1019
Feat/track metrics #1019
Conversation
redisURL := os.Getenv("REDIS_URL") | ||
|
||
if redisURL == "" { | ||
dbInt, _ := utils.ConvertStringToInt(os.Getenv("REDIS_DB")) | ||
RedisClient = redis.NewClient(&redis.Options{ | ||
Addr: os.Getenv("REDIS_HOST"), | ||
Username: os.Getenv("REDIS_USER"), | ||
Password: os.Getenv("REDIS_PASS"), |
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.
these new env vars we might want to add to the README.md or somewhere similar
Price, ok5 := migrateBounty["price"].(uint) | ||
if !ok5 { | ||
migrateBountyFinal.Price = "0" | ||
migrateBountyFinal.Price = 0 |
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.
so I think this only happened when we did that initial migration so I don't think this will modify any existing bounties so they will continue to be strings. Not sure if we should do this change, we should honestly remove this migration code aswell since if we run it again we will have duplicate bounties which is not good
if (newBody.price) { | ||
newBody.price = Number(newBody.price); | ||
} |
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.
This fixes the issue of the bounty price being a string right? not sure if we want to return as a number from the backend so the frontend doesn't have to do this. But that may be a larger change
BountiesPosted int64 `json:"bounties_posted"` | ||
BountiesPaid int64 `json:"bounties_paid"` | ||
BountiesPaidPercentage uint `json:"bounties_paid_average"` | ||
SatsPosted uint `json:"sats_posted"` | ||
SatsPaid uint `json:"sats_paid"` | ||
SatsPaidPercentage uint `json:"sats_paid_percentage"` | ||
AveragePaid uint `json:"average_paid"` | ||
AverageCompleted uint `json:"average_completed"` |
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.
is there a reason why two of them are int64
and the rest are uint
db/redis.go
Outdated
} | ||
|
||
func SetValue(key string, value interface{}) { | ||
err := RedisClient.Set(ctx, key, value, 6*time.Hour).Err() |
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.
can we make 6*time.Hour
a var so it is more obvious incase we want to change that value in the future?
"github.com/stakwork/sphinx-tribes/utils" | ||
) | ||
|
||
func (db database) TotalPeopleByDateRange(r PaymentDateRange) int64 { |
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.
I dont see this being used anywhere
db/metrics.go
Outdated
return 0 | ||
} | ||
|
||
func (db database) CompletedDifferenceSum(r PaymentDateRange) uint { |
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.
I dont see this being used anywhere
db/metrics.go
Outdated
return sum | ||
} | ||
|
||
func (db database) CompletesDifferenceCount(r PaymentDateRange) int64 { |
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.
I dont see this being used anywhere
config/config.go
Outdated
@@ -29,6 +30,9 @@ func InitConfig() { | |||
RelayUrl = os.Getenv("RELAY_URL") | |||
MemeUrl = os.Getenv("MEME_URL") | |||
RelayAuthKey = os.Getenv("RELAY_AUTH_KEY") | |||
AdminStrings := os.Getenv("SUPER_ADMINS") |
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 you can add this ENV var to the README and what it is used for that would be helpful
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.
@elraphty can you change this value to just ADMINS
aswell SUPER_ADMINS
seems a bit much especially since we don't even have the concept of admins implemented yet
Describe your changes
This PR adds the metrics functionality
Issue ticket number and link
Closes #768
Type of change
Checklist before requesting a review