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(logcache-communication): replace the custom LogCacheClient with a simple LogCacheFetcher and build actual logcache.Client in another component #3022

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

geigerj0
Copy link
Contributor

@geigerj0 geigerj0 commented Jun 12, 2024

ℹ️ This PR is based on #2983 ℹ️

Problem

The current implementation of src/autoscaler/eventgenerator/client/log_cache_client.go has some aspects that could benefit from improvement to enhance readability and maintainability:

  • builder like API which is not required
    c := NewLogCacheClient(logger, time.Now, conf.Aggregator.AggregatorExecuteInterval, envelopeProcessor, conf.MetricCollector.MetricCollectorURL)
    if hasUAACreds(conf) {
    uaaCreds := models.UAACreds{
    URL: conf.MetricCollector.UAACreds.URL,
    ClientID: conf.MetricCollector.UAACreds.ClientID,
    ClientSecret: conf.MetricCollector.UAACreds.ClientSecret,
    SkipSSLValidation: conf.MetricCollector.UAACreds.SkipSSLValidation,
    }
    c.SetUAACreds(uaaCreds)
    } else {
    tlsCerts := &models.TLSCerts{
    KeyFile: conf.MetricCollector.TLSClientCerts.KeyFile,
    CertFile: conf.MetricCollector.TLSClientCerts.CertFile,
    CACertFile: conf.MetricCollector.TLSClientCerts.CACertFile,
    }
    tlsConfig, _ := tlsCerts.CreateClientConfig()
    c.SetTLSConfig(tlsConfig)
    }
    c.Configure()
  • building the actual Log Cache should be the responsibility of another component to make it easier testable:
    func (c *LogCacheClient) Configure() {
    var opts []logcache.ClientOption
    if c.uaaCreds.IsEmpty() {
    opts = append(opts, c.goLogCache.WithViaGRPC(c.grpc.WithTransportCredentials(credentials.NewTLS(c.TLSConfig))))
    } else {
    oauth2HTTPOpts := c.goLogCache.WithOauth2HTTPClient(c.getUaaHttpClient())
    oauth2HTTPClient := c.goLogCache.NewOauth2HTTPClient(c.uaaCreds.URL, c.uaaCreds.ClientID, c.uaaCreds.ClientSecret, oauth2HTTPOpts)
    opts = append(opts, c.goLogCache.WithHTTPClient(oauth2HTTPClient))
    }
    c.Client = c.goLogCache.NewClient(c.url, opts...)
    }
  • a lot of unnecessary getters and setters that can be replaced by building the actual Log Cache client with another component:
    func (c *LogCacheClient) SetTLSConfig(tlsConfig *tls.Config) {
    c.TLSConfig = tlsConfig
    }
    func (c *LogCacheClient) GetTlsConfig() *tls.Config {
    return c.TLSConfig
    }
    func (c *LogCacheClient) SetUAACreds(uaaCreds models.UAACreds) {
    c.uaaCreds = uaaCreds
    }
    func (c *LogCacheClient) GetUAACreds() models.UAACreds {
    return c.uaaCreds
    }
    func (c *LogCacheClient) GetUrl() string {
    return c.url
    }
    func (c *LogCacheClient) SetGoLogCache(goLogCache GoLogCache) {
    c.goLogCache = goLogCache
    }
    func (c *LogCacheClient) SetGRPC(grpc GRPC) {
    c.grpc = grpc
    }
  • a lot of unnecessary Log Cache client options that are not needed if the actual Log Cache client is created in another component:
    goLogCache: GoLogCache{
    NewClient: logcache.NewClient,
    WithViaGRPC: logcache.WithViaGRPC,
    WithHTTPClient: logcache.WithHTTPClient,
    NewOauth2HTTPClient: logcache.NewOauth2HTTPClient,
    WithOauth2HTTPClient: logcache.WithOauth2HTTPClient,
    },

Solution

  • replace the whole client/ package with a rewrite in metric/ package
  • introduce a fetcher that has the only responsibility of wrapping the Log Cache client and providing a slim interface to query metrics from Log Cache using the internal client
  • introduce factory that takes care of the actual Log Cache client creation and passes it to a new fetcher

@geigerj0 geigerj0 force-pushed the appautoscaler-703/refactor-logcache-client branch 2 times, most recently from f4b1c75 to 9d590a0 Compare June 12, 2024 21:10
@geigerj0 geigerj0 changed the title refactor(logcache-communication) refactor(logcache-communication): delegate responsibility of Log Cache client creation to another component Jun 12, 2024
@geigerj0 geigerj0 changed the title refactor(logcache-communication): delegate responsibility of Log Cache client creation to another component refactor(logcache-communication): delegate responsibility of Log Cache client creation to dedicated component Jun 12, 2024
@geigerj0 geigerj0 added the chore label Jun 12, 2024
@geigerj0 geigerj0 force-pushed the appautoscaler-703/refactor-logcache-client branch from 9d590a0 to 16a7f2f Compare June 12, 2024 21:24
@geigerj0 geigerj0 changed the title refactor(logcache-communication): delegate responsibility of Log Cache client creation to dedicated component refactor(logcache-communication): replace the custom LogCacheClient with a simple LogCacheFetcher and build actual Log Cache client in another component Jun 12, 2024
@geigerj0 geigerj0 force-pushed the appautoscaler-703/refactor-logcache-client branch 2 times, most recently from 879ecd6 to 8238756 Compare June 12, 2024 21:48
@geigerj0 geigerj0 changed the title refactor(logcache-communication): replace the custom LogCacheClient with a simple LogCacheFetcher and build actual Log Cache client in another component refactor(logcache-communication): replace the custom LogCacheClient with a simple LogCacheFetcher and build actual logcache.Client in another component Jun 12, 2024
@geigerj0 geigerj0 added the allow-acceptance-tests This label needs to be added to enable the acceptance tests to run. label Jun 13, 2024
@geigerj0 geigerj0 marked this pull request as ready for review June 13, 2024 05:26
…onger takes care of also building the actual logcache.Client
Copy link
Contributor

@bonzofenix bonzofenix left a comment

Choose a reason for hiding this comment

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

lgtm

@geigerj0 geigerj0 force-pushed the appautoscaler-703/refactor-logcache-client branch from 8238756 to 3331a67 Compare June 14, 2024 18:29
Base automatically changed from appautoscaler-703/make-logcache-default to main June 17, 2024 09:14
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
13.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@bonzofenix bonzofenix merged commit 8e9794b into main Jun 17, 2024
29 of 30 checks passed
@bonzofenix bonzofenix deleted the appautoscaler-703/refactor-logcache-client branch June 17, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-acceptance-tests This label needs to be added to enable the acceptance tests to run. chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants