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

WIP: Plug datadog apm #101

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

Conversation

cchaudier
Copy link
Contributor

@cchaudier cchaudier commented Aug 3, 2018

Purpose

Add possibility to plug datadog APM to the apps. See #34.

Proposal

We must override the official docker images.
To do this we create a BuildConfig to add datadog APM (ddtrace) on service images.
The DeployementConfig use the OpenShift builded image.
We do this only if th ddtrace ansible vars is set to true.

  • add richie templates
  • add cms templates
  • add lms templates

edit: this PR is depends on #111

fieldRef:
fieldPath: status.hostIP
- name: DD_AGENT_SERVICE_PORT
value: '8126'
Copy link
Contributor

Choose a reason for hiding this comment

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

it should come from a variable?

Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

That's a smart idea 👏

fieldRef:
fieldPath: status.hostIP
- name: DD_AGENT_SERVICE_PORT
value: '8126'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be configurable, e.g. stored in a var?

- name: "richie"
{% if ddtrace is defined and ddtrace %}
image: "docker-registry.default.svc:5000/{{ project_name }}/ddtrace-richie-{{ deployment_stamp }}:{{ richie_image_tag }}"
#image: "ddtrace-richie-{{ deployment_stamp }}:{{ richie_image_tag }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to force docker registry address? Because of minishift?

Anyway, only the commented line should stay right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this with @rouja and propose to always point to an image stream.
Always having an image stream is an optimization we want because it stops downloading all the docker image layers on each deployment.

The syntax is more or less:

template:
    spec:
        containers:
        - name: "richie"
          image: ""
triggers:
    - type: ConfigChange
    - type: ImageChange
      imageChangeParams:
          automatic: true
          containerNames:
          - "richie"
          from:
            kind: ImageStreamTag
            name: "{{ richie_image_name }}:{{ richie_image_tag }}"

Then in the image stream, we build the image we need for each project:

  • adding monitoring (or not)
  • for edxapp: adding a theme (or not)
  • etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a wonderful feature. I'll declare a new issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmaupetit There is already a PR for this ;-) #105

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 yes, but it only scopes edxapp right?

Copy link
Contributor

Choose a reason for hiding this comment

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

True! But it's a functional POC whereas my comment above was a bit vague.

# Switch back to the root user to install development dependencies
USER root:root

RUN pip install --no-cache-dir --prefix=/usr/local ddtrace
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the --prefix is not required: we do not need to split our dependencies in different places.

- name: Set OpenShift datadog objects to manage
set_fact:
images: "{{ templates | map('regex_search', '.*/is.yml.j2$') | select('string') | list }}"
builds: "{{ templates | map('regex_search', '.*/bc.yml.j2$') | select('string') | list }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Those objects should be generic (imagestreams and buidconfigs), not only related to datadog APM. Hence you should move them in the next bloc.

set_fact:
images: "{{ templates | map('regex_search', '.*/is.yml.j2$') | select('string') | list }}"
builds: "{{ templates | map('regex_search', '.*/bc.yml.j2$') | select('string') | list }}"
when: ddtrace
Copy link
Contributor

Choose a reason for hiding this comment

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

We always want to collect those objects not only when ddtrace is true. They are already conditionally created via their templates.

- name: Display OpenShift's build for this app
debug: msg="{{ builds | to_nice_yaml}}"
when: builds is defined
when: ddtrace
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this condition.

tags:
- deploy
- image
- build
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea here: you should create ISs and BCs as generic objects. This should not be datadog-specific.

# ddtrace is set to true for enable the APM in all apps where the templates
# include the possibility to have a ddtrace APM agent
# defaul is set to False
ddtrace: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name it is_apm_enabled instead of ddtrace which is less explicit?


# ddtrace is set to true for enable the APM in all apps where the templates
# include the possibility to have a ddtrace APM agent
# defaul is set to False
Copy link
Contributor

Choose a reason for hiding this comment

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

I would write:

# Set xxx to "true" to enable APM in all apps where defined templates
# include this feature (default: "false")

@jmaupetit jmaupetit added the WIP label Aug 28, 2018
@jmaupetit jmaupetit changed the title Feature/34 plug datadog apm WIP: Plug datadog apm Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants