elastic / apm-agent-python

https://www.elastic.co/guide/en/apm/agent/python/current/index.html
BSD 3-Clause "New" or "Revised" License
413 stars 219 forks source link

Elastic APM support for Minik framework #1779

Open g3kr opened 1 year ago

g3kr commented 1 year ago

Is your feature request related to a problem? Please describe. We are using using minik framework in our AWS lambda based application. Trying to instrument it with Elastic APM doesn't work.

Describe the solution you'd like I understand frameworks like Flask and Django are supported.

Describe alternatives you've considered Are there alternate ways to work around this. Any sample code that can injected to enable instrumentation?

Additional context Elastic APM with python AWS lambda works like a charm. Having issues when integrating with minik framework

basepi commented 1 year ago

Thanks for the request! I haven't spent any time looking into Minik, but I know framework use within serverless contexts is a growing concern. 👍

g3kr commented 1 year ago

Thank you. I have gone through the documentation for creating custom integration and try to reproduce that with minik. Its not straightforward as it looks. If its not too much to ask, do you mind sharing tips to instrument a basic minik app. I can scale it up for my application

from minik.core import Minik

app = Minik()

@app.get("/events")
def get_events():
    return {"data": [{'zip_code': 20902}, {'zip_code': 73071}]}

@app.post("/events")
def post_event():

    event_name = app.request.json_body.get('name')
    # Save this event somewhere
    return {'id': 100, 'name': event_name}

@app.get("/events/{zip_code}")
def get_event(zip_code: int):

    print(f'{type(zip_code)} - {zip_code}')
    if zip_code == 20902:
        return {'events': ['MD Gran fondo', 'Old Busthead']}

    return {'events': ['other events']}`
basepi commented 1 year ago

The big problem with serverless is making sure we don't lose data when the lambda function freezes. You'd probably be better off looking at our @capture_serverless decorator for inspiration instead of that blog post, which is more focused on "normal" applications.

The idea is that you need to begin and end the transaction, and then also call flush on the transport to make sure the transaction is sent to the lambda extension.

But you probably don't want to do this in each of your routes, which is why in other frameworks like Starlette, we use middleware, which allows us to do things before and after each request. Unfortunately Minik does not appear to have any middleware support yet, so to properly instrument it we would need to patch into the underlying library, which can be brittle especially for a project that isn't even 1.0.

I don't have time right now to do a deep dive on Minik's code, but perhaps you could use a simpler version of our @capture_serverless decorator on each of your routes, which could start and end the transaction and also flush the transport? Perhaps something like this:

@app.get("/events")
@capture_minik
def get_events():
    return {"data": [{'zip_code': 20902}, {'zip_code': 73071}]}

If you end up writing a decorator like that, I could probably help if you get stuck.

g3kr commented 1 year ago

@basepi I looked at the @capture_serverless decorator. Its basically instrumenting the lambda function. I tried to use the same in this case and I am seeing the service/lambda show up. However, individual routes do not show up. What do I need to modify for it show up? Will something like this be a starting point for @capture_minik? Your help is appreciated.

import functools
from elasticapm import execution_context, get_client

def capture_minik(func):
    @functools.wraps(func)
    def decorated_function(*args, **kwargs):
        client = get_client()
        if not client.config.instrument:
            return func(*args, **kwargs)

        transaction = client.begin_transaction('Minik')
        execution_context.set_transaction(transaction)

        try:
            response = func(*args, **kwargs)
        except Exception as e:
            client.capture_exception()
            raise e

        if response:
            client.end_transaction(response.status_code)
        else:
            client.end_transaction(500)

        client._transport.flush()

        return response

    return decorated_function
basepi commented 1 year ago

Yes that's actually looking great! I think the missing piece is the transaction name. You can use elasticapm.set_transaction_name() after the transaction has started, and set it to the route name, or function name for the route, or anything else that would be a useful transaction identifier. Right now I'm guessing they're all under a single transaction name, but I can't remember what is used if no transaction name is provided.

g3kr commented 1 year ago

@basepi As suggested by you, I was able to get the instrumentation working by creating a simple @capture_minik decorator.

import functools
import elasticapm
#from elasticapm import get_client

def capture_minik(route):
    def decorator(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            client = elasticapm.Client(service_name="my-minik-service")

            if not client.config.instrument:
                return func(*args, **kwargs)

            transaction = client.begin_transaction('Minik')
            elasticapm.set_transaction_name(route)
            print(f"Capturing metrics for route: {route}")

            try:
                response = func(*args, **kwargs)
            except Exception as e:
                client.capture_exception()
                raise e

            if response:
                print(response)
                client.end_transaction(result="success")
            else:
                client.end_transaction(500)

            client._transport.flush()

            return response

        return wrapper

    return decorator

Now I am trying to get this working for distributed tracing and wondering how to propagate traceparent between 2 lambda function calls instrumented by @capture_minik, so that the service links show up correctly.

With @capture_serverless this was handled automatically. Can you suggest a way to do this with @capture_minik.

basepi commented 1 year ago

You need to get the traceparent from the headers, as we do in @capture_serverless here.

You can then pass that into the begin_transaction() call to link the distributed traces together.