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

Unmarshal identical dedicatedColumns fields less often #3952

Merged
merged 3 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* [CHANGE] Make vParquet4 the default block encoding [#3810](https://github.com/grafana/tempo/pull/3810) (@ie-pham)
* [CHANGE] Azure v2 backend becomes the only and primary Azure backend [#3875](https://github.com/grafana/tempo/pull/3875) (@zalegrala)
**BREAKING CHANGE** The `use_v2_sdk` configuration option has been removed.
* [CHANGE] BlockMeta improvements to reduce the size [#3950](https://github.com/grafana/tempo/pull/3950) [#3951](https://github.com/grafana/tempo/pull/3951) (@zalegrala)
* [CHANGE] BlockMeta improvements to reduce the size [#3950](https://github.com/grafana/tempo/pull/3950) [#3951](https://github.com/grafana/tempo/pull/3951) [#3952](https://github.com/grafana/tempo/pull/3952)(@zalegrala)
* [FEATURE] TraceQL support for link scope and link:traceID and link:spanID [#3741](https://github.com/grafana/tempo/pull/3741) (@stoewer)
* [FEATURE] TraceQL support for link attribute querying [#3814](https://github.com/grafana/tempo/pull/3814) (@ie-pham)
* [FEATURE] TraceQL support for event scope and event:name intrinsic [#3708](https://github.com/grafana/tempo/pull/3708) (@stoewer)
Expand Down
22 changes: 20 additions & 2 deletions tempodb/backend/block_meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@ func (s DedicatedColumnScope) ToTempopb() (tempopb.DedicatedColumn_Scope, error)
}

type CompactedBlockMeta struct {
BlockMeta

CompactedTime time.Time `json:"compactedTime"`
BlockMeta
joe-elliott marked this conversation as resolved.
Show resolved Hide resolved
}

const (
Expand Down Expand Up @@ -174,6 +173,25 @@ func (dc *DedicatedColumn) UnmarshalJSON(b []byte) error {
// DedicatedColumns represents a set of configured dedicated columns.
type DedicatedColumns []DedicatedColumn

func (dcs *DedicatedColumns) UnmarshalJSON(b []byte) error {
// Get the pre-unmarshalled data if available.
v := getDedicatedColumns(string(b))
if v != nil {
*dcs = *v
return nil
}

type dcsAlias DedicatedColumns // alias required to avoid recursive calls of UnmarshalJSON
err := json.Unmarshal(b, (*dcsAlias)(dcs))
if err != nil {
return err
}

putDedicatedColumns(string(b), dcs)

return nil
}

func NewBlockMeta(tenantID string, blockID uuid.UUID, version string, encoding Encoding, dataEncoding string) *BlockMeta {
return NewBlockMetaWithDedicatedColumns(tenantID, blockID, version, encoding, dataEncoding, nil)
}
Expand Down
32 changes: 32 additions & 0 deletions tempodb/backend/json.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package backend

import "sync"

var (
dedicatedColumnsKeeper = map[string]*DedicatedColumns{}
Copy link
Member

Choose a reason for hiding this comment

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

agree there's nothing better than a global var b/c of the way Unmarshal/Marshal works, but i'd like this data to have a lifecycle.

Should we add a "ClearDedicatedColumnsCache()" (or whatever) method that wipes this map called right before polling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I put this in the defer of the poller.Do so that we clear the memory when we're done with polling. I can see putting it at the beginning instead, but then we'd hold the memory even after we're done polling. Could do both I suppose, but this is probably okay.

dedicatedColumnsMtx = sync.Mutex{}
)

func getDedicatedColumns(b string) *DedicatedColumns {
dedicatedColumnsMtx.Lock()
defer dedicatedColumnsMtx.Unlock()

if v, ok := dedicatedColumnsKeeper[b]; ok {
return v
}
return nil
}

func putDedicatedColumns(b string, d *DedicatedColumns) {
dedicatedColumnsMtx.Lock()
defer dedicatedColumnsMtx.Unlock()

dedicatedColumnsKeeper[b] = d
}

func ClearDedicatedColumns() {
dedicatedColumnsMtx.Lock()
defer dedicatedColumnsMtx.Unlock()

clear(dedicatedColumnsKeeper)
}
56 changes: 56 additions & 0 deletions tempodb/backend/json_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package backend

import (
"encoding/json"
"fmt"
"testing"

"github.com/stretchr/testify/require"
)

func Test_getDedicatedColumns(t *testing.T) {
tests := []struct {
name string
jsonStr string
expected *DedicatedColumns
}{
{
name: "case1",
jsonStr: `[ {"s": "resource", "n": "namespace"}, {"n": "http.method"}, {"n": "namespace"} ]`,
expected: &DedicatedColumns{
{Scope: "resource", Name: "namespace", Type: "string"},
{Scope: "span", Name: "http.method", Type: "string"},
{Scope: "span", Name: "namespace", Type: "string"},
},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// clear the map so we start with a clean slate
clear(dedicatedColumnsKeeper)

v := getDedicatedColumns(tc.jsonStr)
require.Nil(t, v)

dcs := &DedicatedColumns{}
err := json.Unmarshal([]byte(tc.jsonStr), dcs)
require.NoError(t, err)
require.Equal(t, tc.expected, dcs)

v = getDedicatedColumns(tc.jsonStr)
require.Equal(t, dcs, v)

p1 := fmt.Sprintf("%p", dcs)
p2 := fmt.Sprintf("%p", v)
require.Equal(t, p1, p2)

dcs2 := &DedicatedColumns{}
err = json.Unmarshal([]byte(tc.jsonStr), dcs2)
require.NoError(t, err)
require.Equal(t, tc.expected, dcs)

require.Equal(t, dcs, dcs2)
})
}
}
1 change: 1 addition & 0 deletions tempodb/blocklist/poller.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ func (p *Poller) Do(previous *List) (PerTenant, PerTenantCompacted, error) {
diff := time.Since(start).Seconds()
metricBlocklistPollDuration.Observe(diff)
level.Info(p.logger).Log("msg", "blocklist poll complete", "seconds", diff)
backend.ClearDedicatedColumns()
}()

ctx, cancel := context.WithCancel(context.Background())
Expand Down