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.

Sign up for GitHub

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

Fix returning nulls from /data/views/query if no data in BigQuery #177

Closed
wants to merge 1 commit into from
Closed

Fix returning nulls from /data/views/query if no data in BigQuery #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions views/bigquery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ type ViewershipEventRow struct {

// metric data

ViewCount int64 `bigquery:"view_count"`
PlaytimeMins float64 `bigquery:"playtime_mins"`
ViewCount bigquery.NullInt64 `bigquery:"view_count"`
PlaytimeMins bigquery.NullFloat64 `bigquery:"playtime_mins"`
Comment on lines +83 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Any idea how these can ever be null?

Not that I think we shouldn't make this fix, but these are the most basic fields that should always be there, defaulting to 0, so it kinda looks like a data pipeline problem.

TtffMs bigquery.NullFloat64 `bigquery:"ttff_ms"`
RebufferRatio bigquery.NullFloat64 `bigquery:"rebuffer_ratio"`
ErrorRate bigquery.NullFloat64 `bigquery:"error_rate"`
Expand All @@ -92,9 +92,9 @@ type ViewSummaryRow struct {
PlaybackID bigquery.NullString `bigquery:"playback_id"`
DStorageURL bigquery.NullString `bigquery:"d_storage_url"`

ViewCount int64 `bigquery:"view_count"`
LegacyViewCount bigquery.NullInt64 `bigquery:"legacy_view_count"`
PlaytimeMins float64 `bigquery:"playtime_mins"`
ViewCount bigquery.NullInt64 `bigquery:"view_count"`
LegacyViewCount bigquery.NullInt64 `bigquery:"legacy_view_count"`
PlaytimeMins bigquery.NullFloat64 `bigquery:"playtime_mins"`
}

type BigQuery interface {
Expand Down
18 changes: 11 additions & 7 deletions views/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@

// metric data

ViewCount int64 `json:"viewCount"`
PlaytimeMins float64 `json:"playtimeMins"`
ViewCount data.Nullable[int64] `json:"viewCount,omitempty"`
PlaytimeMins data.Nullable[float64] `json:"playtimeMins,omitempty"`
TtffMs data.Nullable[float64] `json:"ttffMs,omitempty"`
RebufferRatio data.Nullable[float64] `json:"rebufferRatio,omitempty"`
ErrorRate data.Nullable[float64] `json:"errorRate,omitempty"`
Expand Down Expand Up @@ -97,7 +97,7 @@

return []TotalViews{{
ID: asset.PlaybackID,
StartViews: startViews,
StartViews: data.ToNullable[int64](startViews, true, true),

Check warning on line 100 in views/client.go

View check run for this annotation

Codecov / codecov/patch

views/client.go#L100 

Added line #L100 was not covered by tests
}}, nil
}

Expand Down Expand Up @@ -126,9 +126,9 @@
return &Metric{
PlaybackID: toStringPtr(summary.PlaybackID, summary.PlaybackID.Valid),
DStorageURL: toStringPtr(summary.DStorageURL, summary.DStorageURL.Valid),
ViewCount: summary.ViewCount,
ViewCount: toInt64Ptr(summary.ViewCount, summary.ViewCount.Valid),

Check warning on line 129 in views/client.go

View check run for this annotation

Codecov / codecov/patch

views/client.go#L129 

Added line #L129 was not covered by tests
LegacyViewCount: data.ToNullable[int64](legacyViewCount, true, true),
PlaytimeMins: summary.PlaytimeMins,
PlaytimeMins: toFloat64Ptr(summary.PlaytimeMins, summary.PlaytimeMins.Valid),

Check warning on line 131 in views/client.go

View check run for this annotation

Codecov / codecov/patch

views/client.go#L131 

Added line #L131 was not covered by tests
}
}

Expand Down Expand Up @@ -187,8 +187,8 @@
Subdivision: toStringPtr(row.Subdivision, spec.hasBreakdownBy("subdivision")),
TimeZone: toStringPtr(row.TimeZone, spec.hasBreakdownBy("timezone")),
GeoHash: toStringPtr(row.GeoHash, spec.hasBreakdownBy("geohash")),
ViewCount: row.ViewCount,
PlaytimeMins: row.PlaytimeMins,
ViewCount: toInt64Ptr(row.ViewCount, spec.Detailed),
PlaytimeMins: toFloat64Ptr(row.PlaytimeMins, spec.Detailed),

Check warning on line 191 in views/client.go

View check run for this annotation

Codecov / codecov/patch

views/client.go#L190-L191 

Added lines #L190 - L191 were not covered by tests
Comment on lines +190 to +191
Copy link
Member

@victorges victorges Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These 2 fields are actually not detailed, they belong to the basic viewership metrics and should always be in the response. So they are always "asked".

Suggested change
ViewCount: toInt64Ptr(row.ViewCount, spec.Detailed),
PlaytimeMins: toFloat64Ptr(row.PlaytimeMins, spec.Detailed),
ViewCount: toInt64Ptr(row.ViewCount, true),
PlaytimeMins: toFloat64Ptr(row.PlaytimeMins, true),

(otherwise if you call the API without ?detailed=true you will get NO metrics on the response!)

Comment on lines +190 to +191
Copy link
Member

@victorges victorges Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking further about this, we probably don't want to return null on the API responses for these fields, as we shouldn't leak the datasource bug to the end-user. So IMO these fields should always be present and instead we return 0 if they are null, WDYT? We could fix this directly on the SQL queries, like adding a COALESCE on those fields, or we could just have a logic here to check v == null ? 0 : v.

TtffMs: toFloat64Ptr(row.TtffMs, spec.Detailed),
RebufferRatio: toFloat64Ptr(row.RebufferRatio, spec.Detailed),
ErrorRate: toFloat64Ptr(row.ErrorRate, spec.Detailed),
Expand All @@ -205,6 +205,10 @@
return metrics
}

func toInt64Ptr(bqInt bigquery.NullInt64, asked bool) data.Nullable[int64] {
return data.ToNullable(bqInt.Int64, bqInt.Valid, asked)

Check warning on line 209 in views/client.go

View check run for this annotation

Codecov / codecov/patch

views/client.go#L208-L209 

Added lines #L208 - L209 were not covered by tests
}

func toFloat64Ptr(bqFloat bigquery.NullFloat64, asked bool) data.Nullable[float64] {
return data.ToNullable(bqFloat.Float64, bqFloat.Valid, asked)
}
Expand Down
5 changes: 3 additions & 2 deletions views/prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (

"github.com/golang/glog"
livepeer "github.com/livepeer/go-api-client"
"github.com/livepeer/livepeer-data/pkg/data"
promClient "github.com/prometheus/client_golang/api"
prometheus "github.com/prometheus/client_golang/api/prometheus/v1"
"github.com/prometheus/common/model"
)

type TotalViews struct {
ID string `json:"id"`
StartViews int64 `json:"startViews"`
ID string `json:"id"`
StartViews data.Nullable[int64] `json:"startViews"`
}

type Prometheus struct {
Expand Down
Loading