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: Rename for clarity in an E&S world #680

Merged
merged 23 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from 22 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 collect/collect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ func TestDryRunMode(t *testing.T) {
GetSamplerTypeVal: &config.DeterministicSamplerConfig{
SampleRate: 10,
},
SendTickerVal: 2 * time.Millisecond,
SendTickerVal: 20 * time.Millisecond,
TylerHelmuth marked this conversation as resolved.
Show resolved Hide resolved
DryRun: true,
DryRunFieldName: field,
ParentIdFieldNames: []string{"trace.parent_id", "parentId"},
Expand Down
7 changes: 4 additions & 3 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,11 @@ type Config interface {
// GetInMemCollectorCacheCapacity returns the config specific to the InMemCollector
GetInMemCollectorCacheCapacity() (InMemoryCollectorCacheCapacity, error)

// GetSamplerConfigForDataset returns the sampler type and name to use for the given dataset
GetSamplerConfigForDataset(string) (interface{}, string, error)
// GetSamplerConfigForDestName returns the sampler type and name to use for
// the given destination (environment, or dataset in classic)
GetSamplerConfigForDestName(string) (interface{}, string, error)

// GetAllSamplerRules returns all dataset rules in a map, including the default
// GetAllSamplerRules returns all rules in a single map, including the default rules
GetAllSamplerRules() (map[string]interface{}, error)

// GetMetricsType returns the type of metrics to use. Valid types are in the
Expand Down
4 changes: 2 additions & 2 deletions config/configLoadHelpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,9 @@ func readConfigInto(dest any, location string, opts *CmdEnv) (string, error) {
return hash, nil
}

// reloadInto accepts a map[string]any and a struct, and loads the map into the struct
// ReloadInto accepts a map[string]any and a struct, and loads the map into the struct
// by re-marshalling the map into JSON and then unmarshalling the JSON into the struct.
func reloadInto(m map[string]any, dest interface{}, opts *CmdEnv) error {
func ReloadInto(m map[string]any, dest interface{}, opts *CmdEnv) error {
kentquirk marked this conversation as resolved.
Show resolved Hide resolved
b, err := json.Marshal(m)
if err != nil {
return fmt.Errorf("reloadInto unable to marshal config: %w", err)
Expand Down
22 changes: 11 additions & 11 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ func TestReadDefaults(t *testing.T) {
t.Error("received", d, "expected", time.Hour)
}

d, name, err := c.GetSamplerConfigForDataset("dataset-doesnt-exist")
d, name, err := c.GetSamplerConfigForDestName("dataset-doesnt-exist")
assert.NoError(t, err)
assert.IsType(t, &DeterministicSamplerConfig{}, d)
assert.Equal(t, "DeterministicSampler", name)
Expand All @@ -292,17 +292,17 @@ func TestReadRulesConfig(t *testing.T) {
c, err := getConfig([]string{"--config", "../config.toml", "--rules_config", "../rules_complete.toml"})
assert.NoError(t, err)

d, name, err := c.GetSamplerConfigForDataset("dataset-doesnt-exist")
d, name, err := c.GetSamplerConfigForDestName("dataset-doesnt-exist")
assert.NoError(t, err)
assert.IsType(t, &DeterministicSamplerConfig{}, d)
assert.Equal(t, "DeterministicSampler", name)

d, name, err = c.GetSamplerConfigForDataset("dataset1")
d, name, err = c.GetSamplerConfigForDestName("dataset1")
assert.NoError(t, err)
assert.IsType(t, &DynamicSamplerConfig{}, d)
assert.Equal(t, "DynamicSampler", name)

d, name, err = c.GetSamplerConfigForDataset("dataset4")
d, name, err = c.GetSamplerConfigForDestName("dataset4")
assert.NoError(t, err)
switch r := d.(type) {
case *RulesBasedSamplerConfig:
Expand Down Expand Up @@ -393,7 +393,7 @@ func TestAbsentTraceKeyField(t *testing.T) {
defer os.Remove(config)
cfg, err := getConfig([]string{"--config", config, "--rules_config", rules})
assert.NoError(t, err)
_, samplerName, err := cfg.GetSamplerConfigForDataset("dataset1")
_, samplerName, err := cfg.GetSamplerConfigForDestName("dataset1")
assert.Error(t, err)
assert.Equal(t, "EMADynamicSampler", samplerName)
assert.Contains(t, err.Error(), "Error:Field validation for 'AddSampleRateKeyToTraceField'")
Expand Down Expand Up @@ -517,27 +517,27 @@ func TestGetSamplerTypes(t *testing.T) {
c, err := getConfig([]string{"--config", config, "--rules_config", rules})
assert.NoError(t, err)

if d, name, err := c.GetSamplerConfigForDataset("dataset-doesnt-exist"); assert.Equal(t, nil, err) {
if d, name, err := c.GetSamplerConfigForDestName("dataset-doesnt-exist"); assert.Equal(t, nil, err) {
assert.IsType(t, &DeterministicSamplerConfig{}, d)
assert.Equal(t, "DeterministicSampler", name)
}

if d, name, err := c.GetSamplerConfigForDataset("dataset 1"); assert.Equal(t, nil, err) {
if d, name, err := c.GetSamplerConfigForDestName("dataset 1"); assert.Equal(t, nil, err) {
assert.IsType(t, &DynamicSamplerConfig{}, d)
assert.Equal(t, "DynamicSampler", name)
}

if d, name, err := c.GetSamplerConfigForDataset("dataset2"); assert.Equal(t, nil, err) {
if d, name, err := c.GetSamplerConfigForDestName("dataset2"); assert.Equal(t, nil, err) {
assert.IsType(t, &DeterministicSamplerConfig{}, d)
assert.Equal(t, "DeterministicSampler", name)
}

if d, name, err := c.GetSamplerConfigForDataset("dataset3"); assert.Equal(t, nil, err) {
if d, name, err := c.GetSamplerConfigForDestName("dataset3"); assert.Equal(t, nil, err) {
assert.IsType(t, &EMADynamicSamplerConfig{}, d)
assert.Equal(t, "EMADynamicSampler", name)
}

if d, name, err := c.GetSamplerConfigForDataset("dataset4"); assert.Equal(t, nil, err) {
if d, name, err := c.GetSamplerConfigForDestName("dataset4"); assert.Equal(t, nil, err) {
assert.IsType(t, &TotalThroughputSamplerConfig{}, d)
assert.Equal(t, "TotalThroughputSampler", name)
}
Expand All @@ -560,7 +560,7 @@ func TestDefaultSampler(t *testing.T) {

assert.NoError(t, err)

s, name, err := c.GetSamplerConfigForDataset("nonexistent")
s, name, err := c.GetSamplerConfigForDestName("nonexistent")

assert.NoError(t, err)
assert.Equal(t, "DeterministicSampler", name)
Expand Down
4 changes: 2 additions & 2 deletions config/config_test_reload_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestErrorReloading(t *testing.T) {
c, err := NewConfig(opts, func(err error) { ch <- 1 })
assert.NoError(t, err)

d, name, _ := c.GetSamplerConfigForDataset("dataset5")
d, name, _ := c.GetSamplerConfigForDestName("dataset5")
if _, ok := d.(DeterministicSamplerConfig); ok {
t.Error("type received", d, "expected", "DeterministicSampler")
}
Expand Down Expand Up @@ -83,7 +83,7 @@ func TestErrorReloading(t *testing.T) {
wg.Wait()

// config should error and not update sampler to invalid type
d, _, _ = c.GetSamplerConfigForDataset("dataset5")
d, _, _ = c.GetSamplerConfigForDestName("dataset5")
if _, ok := d.(DeterministicSamplerConfig); ok {
t.Error("received", d, "expected", "DeterministicSampler")
}
Expand Down
21 changes: 11 additions & 10 deletions config/file_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func newFileConfig(opts *CmdEnv) (*fileConfig, error) {
}

// Run a basic validation on the sampler config; we can do better after a reorganization of this.
if _, _, err := cfg.GetSamplerConfigForDataset("**invalid dataset name**"); err != nil {
if _, _, err := cfg.GetSamplerConfigForDestName("**invalid dataset name**"); err != nil {
return nil, err
}

Expand Down Expand Up @@ -458,22 +458,23 @@ func getValueForCaseInsensitiveKey[T any](m map[string]any, key string, def T) (
return def, false
}

// GetSamplerConfigForDataset returns the sampler config for the given dataset,
// as well as the name of the sampler. If the dataset-specific sampler config
// is not found, it returns the default sampler config.
func (f *fileConfig) GetSamplerConfigForDataset(dataset string) (any, string, error) {
// GetSamplerConfigForDestName returns the sampler config for the given
// destination (environment, or dataset in classic mode), as well as the name of
// the sampler. If the specific sampler config is not found, it returns the
// default sampler config.
func (f *fileConfig) GetSamplerConfigForDestName(destname string) (any, string, error) {
f.mux.RLock()
defer f.mux.RUnlock()

config := f.rulesConfig
// If we have a dataset-specific sampler, we extract the sampler config
// corresponding to the [dataset]["sampler"] key. Otherwise we try to use
// If we have a specific sampler, we extract the sampler config
// corresponding to the [destname]["sampler"] key. Otherwise we try to use
// the default sampler config corresponding to the "sampler" key. Only if
// both fail will we return not found.

const notfound = "not found"
if v, ok := getValueForCaseInsensitiveKey(config, dataset, map[string]any{}); ok {
// we have a dataset-specific sampler, so we extract that sampler's config
if v, ok := getValueForCaseInsensitiveKey(config, destname, map[string]any{}); ok {
// we have a specific sampler, so we extract that sampler's config
config = v
}

Expand All @@ -497,7 +498,7 @@ func (f *fileConfig) GetSamplerConfigForDataset(dataset string) (any, string, er
}

// now we need to unmarshal the config into the sampler config struct
err := reloadInto(config, i, f.opts)
err := ReloadInto(config, i, f.opts)
return i, samplerName, err
}

Expand Down
2 changes: 1 addition & 1 deletion config/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func (m *MockConfig) GetMaxBatchSize() uint {
}

// TODO: allow per-dataset mock values
func (m *MockConfig) GetSamplerConfigForDataset(dataset string) (interface{}, string, error) {
func (m *MockConfig) GetSamplerConfigForDestName(dataset string) (interface{}, string, error) {
m.Mux.RLock()
defer m.Mux.RUnlock()

Expand Down
2 changes: 1 addition & 1 deletion route/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ func (r *Router) debugTrace(w http.ResponseWriter, req *http.Request) {
func (r *Router) getSamplerRules(w http.ResponseWriter, req *http.Request) {
format := strings.ToLower(mux.Vars(req)["format"])
dataset := mux.Vars(req)["dataset"]
cfg, name, err := r.Config.GetSamplerConfigForDataset(dataset)
cfg, name, err := r.Config.GetSamplerConfigForDestName(dataset)
if err != nil {
w.Write([]byte(fmt.Sprintf("got error %v trying to fetch config for dataset %s\n", err, dataset)))
w.WriteHeader(http.StatusBadRequest)
Expand Down
2 changes: 1 addition & 1 deletion sample/sample.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func (s *SamplerFactory) GetSamplerImplementationForKey(samplerKey string, isLeg
}
}

c, _, err := s.Config.GetSamplerConfigForDataset(samplerKey)
c, _, err := s.Config.GetSamplerConfigForDestName(samplerKey)
if err != nil {
return nil
}
Expand Down