making topology use config type and adding alb to topology #155

jhsinger-klotho · 2023-01-30T16:39:51Z

• Does any part of it require special attention?
• Does it relate to or fix any issue?

closes #111

topology will get type from config since that is the source of truth. Added alb to the topology diagrams and validated it works.

Some small infra changes to the ALB code as well, will change some integ tests to use ALB so we can properly test it

We now remove Type() from all Cloud Resources so we have a single source of truth. This required me to bundle in the telemetry changes as well requested by ala since we report type up to datadog/mixpanel

Standard checks

Unit tests: Any special considerations?
Docs: Do we need to update any docs, internal or public?
Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working?

gordon-klotho · 2023-01-30T16:41:45Z

pkg/topology/topology.go

@@ -66,7 +66,7 @@ func (p Plugin) generateIconsAndEdges(resource core.CloudResource, dependencies
 		Title: resource.Key().Name,
 		Image: p.getImagePath(resource),
 		Kind:  resource.Key().Kind,
-		Type:  resource.Type(),
+		Type:  p.Config.GetResourceType(resource),


I'm not a fan of this change because it's conceding that resource.Type() is incorrect. If we're not going to use it, then it needs to be removed.

resource.Type is incorrect and should be removed. This will get our integ tests passing though which is why i want to get it in

i can cut a new ticket to remove resource.Type if we want

That means that plugins will not be empowered to determine the type on the resources it creates. It was originally intended for cases such as if none was specified, it could decide to use something other than the default - like something more intelligent. One example might be to use an ALB instead of API Gateway for an expose if all connecting exec units were non-lambda.

Something to consider, but not a blocker. Perhaps we can discuss quickly in sync.

An alternative would be to fix the plugins to properly populate the type (or have some process at the end filling in from the config if none was specified).

lets discuss in sync, but i dont see it making much sense to store the same type in 2 places because how do we know what is the source of truth. Plugins all have access to the application config so that is what i think should be the source of truth

fixing the plugins doesnt make sense to me. We could have 100 expose plugins so that means each one has to hardcode -> Get type from config, set resource type. We already have config so why not use it, plugins should be able to set the config type if they want to make the change

Just noting here - I was convinced during sync that changing the config was the better and proper approach. For posterity: the primary driving factor is in making sure the config as we output to the compiled directory needs to match the resources we create. Having the type on the resource would not accomplish this, and if we have to set it on the config anyway, there's no point having a duplicate somewhere else.

gordon-klotho · 2023-01-30T16:41:55Z

pkg/core/topology.go

@@ -62,7 +62,8 @@ var DiagramEntityToImgPath = TopoMap{
 	{Kind: PubSubKind}:                      "generic/blank/blank.png",

 	// Use AWS as the ultimate fallback for the Kind, so don't specify Provider.
-	{Kind: GatewayKind, Provider: ProviderAWS}:                          "aws/network/api-gateway.png",
+	{Kind: GatewayKind, Provider: ProviderAWS, Type: "apigateway"}:      "aws/network/api-gateway.png",
+	{Kind: GatewayKind, Provider: ProviderAWS, Type: "alb"}:             "aws/network/elb-application-load-balancer.png",


ghost · 2023-01-31T18:18:38Z

pkg/cli/helpers.go

 	for _, res := range result.Resources() {
 		switch r := res.(type) {
 		case *core.ExecutionUnit:
-			executableLangs[r.Executable.Type] = true
+			executableLangs = append(executableLangs, r.Executable.Type)
 		}
 	}
 	return executableLangs


If we're turning this to an array anyway, is it worth sorting it so that it's stable across runs? (Even if we don't care what the order is, just to have it be the same.)

this is purely for internal metrics, so i dont think its necessary. We just send this up to mixpanel and then should write logic to sum the number of langs used acrodss apps

ghost · 2023-01-31T18:19:05Z

pkg/cli/helpers.go

-			resourceCounts[res.Key().Kind] = make(map[string]int)
-		}
-		resourceCounts[res.Key().Kind][res.Type()]++
+		resourceCounts = append(resourceCounts, cfg.GetResourceType(res))


ditto the sorting question

same as above

gordon-klotho

Nothing blocking, but I do hope addressing the comment on the resource count function can get squeezed in.

gordon-klotho · 2023-01-31T18:27:38Z

pkg/cli/helpers.go

 		}
 	}
 	return executableLangs
 }

-func GetResourceTypeCount(result *core.CompilationResult) (resourceCounts map[string]map[string]int) {
-	resourceCounts = make(map[string]map[string]int)
+func GetResourceTypeCount(result *core.CompilationResult, cfg *config.Application) (resourceCounts []string) {


This doesn't return the counts anymore. I'd be fine w/ either adjusting the name to match, or reverting back to returning the counts.
I don't think iteration order for this matters since it's only used internally and not in any outputs.

yeah so this was a request from ala because mixpanel doesnt process it. The hope here was that we have a list which contains each instance of a type so we can sum the number of "eks" in that list. If youve got a better way to approach this let me know, just needs to work within mixpanels confinements

So something like {"eks": 2, "apigateway": 1} doesn't work and we'd want ["eks", "eks", "apigateway"] instead? In that case, this is fine - could you update the function name to something to reflect its new return (eg GetResourceTypes)?

gordon-klotho · 2023-01-31T18:28:16Z

pkg/cli/klothomain.go

@@ -218,7 +218,7 @@ func (km KlothoMain) run(cmd *cobra.Command, args []string) (err error) {
 	}
 	klothoName := "klotho"
 	if km.VersionQualifier != "" {
-		klothoName += " " + km.VersionQualifier
+		analyticsClient.Properties[km.VersionQualifier] = true


Not sure what this change is for, seems unrelated?

our metrics in mixpanel are screwed up because it says "klotho procompile complete", etc. This should make it consistent across versions saying "klotho compile complete" with a pro flag so we know if theyre on oss or pro

gordon-klotho · 2023-01-31T18:29:18Z

pkg/core/exec_unit.go

@@ -74,8 +74,6 @@ var (
 	ExecutableTypeGolang = ExecutableType("Golang")
 )

-func (unit *ExecutionUnit) Type() string { return unit.ExecType }


Should ExecType (the field) also be removed?

good catch, will go remove

gordon-klotho · 2023-01-31T18:32:45Z

pkg/topology/topology.go

@@ -66,7 +66,7 @@ func (p Plugin) generateIconsAndEdges(resource core.CloudResource, dependencies
 		Title: resource.Key().Name,
 		Image: p.getImagePath(resource),
 		Kind:  resource.Key().Kind,
-		Type:  resource.Type(),
+		Type:  p.Config.GetResourceType(resource),


Just noting here - I was convinced during sync that changing the config was the better and proper approach. For posterity: the primary driving factor is in making sure the config as we output to the compiled directory needs to match the resources we create. Having the type on the resource would not accomplish this, and if we have to set it on the config anyway, there's no point having a duplicate somewhere else.

github-actions · 2023-01-31T19:19:49Z

Package	Line Rate	Health
github.com/klothoplatform/klotho/pkg/analytics	2%	❌
github.com/klothoplatform/klotho/pkg/annotation	23%	❌
github.com/klothoplatform/klotho/pkg/cli	4%	❌
github.com/klothoplatform/klotho/pkg/core	21%	❌
github.com/klothoplatform/klotho/pkg/env_var	82%	✔
github.com/klothoplatform/klotho/pkg/exec_unit	54%	➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes	59%	➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm	39%	❌
github.com/klothoplatform/klotho/pkg/input	63%	➖
github.com/klothoplatform/klotho/pkg/lang	37%	❌
github.com/klothoplatform/klotho/pkg/lang/dockerfile	0%	❌
github.com/klothoplatform/klotho/pkg/lang/golang	20%	❌
github.com/klothoplatform/klotho/pkg/lang/javascript	47%	❌
github.com/klothoplatform/klotho/pkg/lang/python	61%	➖
github.com/klothoplatform/klotho/pkg/lang/yaml	0%	❌
github.com/klothoplatform/klotho/pkg/logging	7%	❌
github.com/klothoplatform/klotho/pkg/multierr	95%	✔
github.com/klothoplatform/klotho/pkg/provider/aws	59%	➖
github.com/klothoplatform/klotho/pkg/runtime	75%	✔
github.com/klothoplatform/klotho/pkg/static_unit	33%	❌
github.com/klothoplatform/klotho/pkg/validation	73%	➖
github.com/klothoplatform/klotho/pkg/yaml_util	79%	✔
Summary	40% (3799 / 9453)	❌

gordon-klotho requested changes Jan 30, 2023

View reviewed changes

Jordan Singer added 2 commits January 31, 2023 10:25

making topology use config type and adding alb to topology

06c723f

remove type from resources

ec393b8

jhsinger-klotho force-pushed the config_type branch from 3756657 to ec393b8 Compare January 31, 2023 16:38

Jordan Singer added 3 commits January 31, 2023 11:17

listener conditions need to be a list

9878a41

needs to be uppercased

7fe4f24

albs are finicky with their paths

341fdfd

ghost approved these changes Jan 31, 2023

View reviewed changes

gordon-klotho approved these changes Jan 31, 2023

View reviewed changes

Jordan Singer added 2 commits January 31, 2023 13:08

remove exectype and add config to expose plugins

68c5235

dont count internal resources with no type

bb479d6

jhsinger-klotho merged commit 91e8f01 into main Jan 31, 2023

jhsinger-klotho deleted the config_type branch January 31, 2023 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

making topology use config type and adding alb to topology #155

making topology use config type and adding alb to topology #155

jhsinger-klotho commented Jan 30, 2023 •

edited

Loading

gordon-klotho Jan 30, 2023

jhsinger-klotho Jan 30, 2023

jhsinger-klotho Jan 30, 2023

gordon-klotho Jan 30, 2023 •

edited

Loading

jhsinger-klotho Jan 30, 2023

jhsinger-klotho Jan 30, 2023

gordon-klotho Jan 31, 2023

gordon-klotho Jan 30, 2023

ghost Jan 31, 2023

jhsinger-klotho Jan 31, 2023

ghost Jan 31, 2023

jhsinger-klotho Jan 31, 2023

gordon-klotho left a comment

gordon-klotho Jan 31, 2023

jhsinger-klotho Jan 31, 2023

gordon-klotho Jan 31, 2023

gordon-klotho Jan 31, 2023

jhsinger-klotho Jan 31, 2023

gordon-klotho Jan 31, 2023

jhsinger-klotho Jan 31, 2023

gordon-klotho Jan 31, 2023

github-actions bot commented Jan 31, 2023

making topology use config type and adding alb to topology #155

making topology use config type and adding alb to topology #155

Conversation

jhsinger-klotho commented Jan 30, 2023 • edited Loading

Standard checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordon-klotho Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordon-klotho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2023

jhsinger-klotho commented Jan 30, 2023 •

edited

Loading

gordon-klotho Jan 30, 2023 •

edited

Loading