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

EMF as Map #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

EMF as Map #12

wants to merge 4 commits into from

Conversation

prozz
Copy link
Owner

@prozz prozz commented Aug 10, 2023

Hey @gaeste!

I noticed you had the need to introduce EMF datastructure exposure, instead of logging.
Could you please share your requirements for it and how is it used in practice?

Regards :)

gaeste added 4 commits July 3, 2023 14:07
* Adds Build() to expose the EMF structure externally

* bumped go version to 1.20
* Update pipeline actions
* go 1.20 update
* Update pipeline actions
* go 1.20 update
@gaeste
Copy link

gaeste commented Apr 23, 2024

Hey @prozz!
Sorry for not getting back to you.

We're having a log implementation with a fluent interface where we added a withMetrics(m emf.Metrics) kind of operation. But in order to use this library - which has most of what we need, so kudos! - we wanted to take the emf-part and build it in order to use our log implementations fluent interface.

Something in the lines of:

	metrics := emf.New().
		Namespace("MyNamespace").
		Dimension("aDimension", "dim1").
		MetricAs("counter", 1, emf.Count).
		Build()

	log.WithMetrics(metrics).With("named_key", "a_value").Info("this is my log message including emf")

It is just an enabler for us to use this library in multiple ways.
Hope it helps 👍

@jsvensson
Copy link

Hi! I work with @gaeste and just came across this. I'm working on our internal log package and I was looking at dependencies, which is how I ended up here.

Our use case is that we have, as mentioned, our own package for logging. It's due for some overhaul, and one new feature is proper JSON support, which we need for EMF. EMF in general is new to me, so I'm still getting my bearings on it.

Our log package has a .With("key", val) function to add structured data. Apart from primitives, it can take a map and marshal it to JSON and add that to the log. We have a mildly hacky .WithMetrics() that takes the EMF map, ensures that it contais the _aws key, and inserts the values. That's the main reason for needing the map directly.

(mildly edited for brevity)

func WithMetrics(emf map[string]any)Logger {
	metrics := emf["_aws"]
	if metrics == nil {
		return log.With("emf_error", "no EMF structure available")
	}

	logger := log.With("_aws", metrics)
	for k, v := range emf {
		if k != "_aws" {
			logger = logger.With(k, v)
		}
	}

	return logger
}

However, just slinging bare maps around can leave room for mistakes when you want a specific structure to it, and you need to actually check the logs for that emf_error to spot any issues. I think it would be preferrable to have the EMF package handle all of that instead, maybe with an actual EMF type.

...so that was a long-winded way of saying that we're not using the logger part of this package, but we didn't want to reinvent the wheel for the EMF format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants