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

Track mapconfig id / template id in logs #1135

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

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented Oct 29, 2019

@simon-contreras-deel simon-contreras-deel marked this pull request as ready for review October 29, 2019 17:48
Copy link
Contributor

@dgaubert dgaubert left a comment

Choose a reason for hiding this comment

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

Why do we need this?

In Kibana we already have this info. Check layergroup_id and named_map_template_id fields.

@simon-contreras-deel
Copy link
Contributor Author

The idea is to add more info to follow all the mapconfig flow:

  • know where the map config was created or instantiated
  • know where a template was created and instantiated
  • know the relation between template and mapconfig

Makes sense?

@dgaubert
Copy link
Contributor

know where the map config was created or instantiated
know where a template was created and instantiated

Do you mean the Maps API instance? If so, we already have that info. See: event_source_node field.

know the relation between template and map-config

That's more interesting. 🤔

Copy link
Contributor

@dgaubert dgaubert left a comment

Choose a reason for hiding this comment

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

@simon-contreras-deel
Copy link
Contributor Author

Let me explain better:

  • know in which request the map config was created or instantiated (now we don't know that, it is important to know when the mapconfig is created)
  • know in which request a template was created and instantiated
  • know the relation between template id and mapconfig id

With all this info, we can know the lifecycle of a mapconfig / template. The idea is to get more info to investigate lost mapconfigs.

I know I have added template id / mapconfig id in some places where we already have it (for example in the template update or delete where we get them from the URL) but I want to have a consistent place to have the info.

@dgaubert
Copy link
Contributor

Yeah, I know what you want to accomplish here. 😄

Apart from the links that I shared above, which give you most of what you need to be able to add it in the log entry, you'll have to add a new header with the template id (X-Template-Id) when the template is created. Once you have it you need to use those headers to add them in the log entry.

See: https://github.com/CartoDB/Windshaft-cartodb/blob/master/lib/api/template/admin-template-controller.js#L135-L144

Copy link
Contributor

@dgaubert dgaubert left a comment

Choose a reason for hiding this comment

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

Now I understand better what you want to achieve. I left a couple of comments.

lib/api/middlewares/logging-info.js Outdated Show resolved Hide resolved
lib/api/api-router.js Outdated Show resolved Hide resolved
lib/api/middlewares/logging-info.js Outdated Show resolved Hide resolved
lib/api/middlewares/tiler-info-header.js Show resolved Hide resolved
@@ -82,6 +82,7 @@ function getTile ({ tileBackend, label }) {

res.statusCode = 200;
res.body = tile;
res.locals.mapConfig = mapConfigProvider.mapConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need it, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is the only way I found the get the relation between template and mapconfig. But I see now it is possible using the logic from setLayergroupIdHeaderMiddleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I explained in the other comment, it seems more complicated to use the logic from setLayergroupIdHeaderMiddleware instead of saving mapconfig in locals (or simply the id if you see any memory problem)

Copy link
Contributor

Choose a reason for hiding this comment

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

you already have the layergroup token in multiple places:

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was mixing controllers we are in template.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should use mapConfigProvider.getMapConfig() method instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, why not move this to setTilerInfoHeader middleware as mapConfigProvider is already set in res.locals which is available in that middleware?.

Copy link
Contributor Author

@simon-contreras-deel simon-contreras-deel Nov 6, 2019

Choose a reason for hiding this comment

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

Ok, I am going to extract the mapconfig id from mapConfigProvider in setTilerInfoHeader

You should use mapConfigProvider.getMapConfig() method instead.

As setTilerInfoHeader happens after all the controller middlewares, if everything goes fine, mapConfigProvider should already have the mapConfig property set and we can avoid doing a new redis request.

Copy link
Contributor

Choose a reason for hiding this comment

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

lib/api/middlewares/named-map-provider.js Outdated Show resolved Hide resolved
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.

2 participants